Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Related posts improve markup and add alt text #11324

Merged
merged 2 commits into from
Mar 26, 2019
Merged

Related posts improve markup and add alt text #11324

merged 2 commits into from
Mar 26, 2019

Conversation

aldavigdis
Copy link
Contributor

@aldavigdis aldavigdis commented Feb 11, 2019 •
edited
Loading

This is a follow-up PR to #11220 and includes commits from there as well. Please merge that before this one.

Fixes #11175

Changes proposed in this Pull Request:

  • Adding support for alt text to Jetpack_PostImages
  • Using alt text for images instead of a redundant article title
  • Using sprintf to output HTML markup without newlines
  • Properly using nav and ul elements to indicate related posts, increasing a11y
  • Removes has-small-font-size classes from some HTML elements as this was causing a discrepancy between the editor (where this is defined) and the fronted, where is depends on the theme.

Testing instructions:

  • See if the markup in the Related Posts block breaks in any way

Proposed changelog entry for your changes:

  • Improved HTML markup for related posts, with emphasis on accessibility

@aldavigdis aldavigdis added [Feature] Related Posts [Focus] Accessibility Improving usability for all users (a11y) [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Feb 11, 2019
@aldavigdis aldavigdis requested review from lezama and a team February 11, 2019 23:19
@jetpackbot
Copy link
Collaborator

jetpackbot commented Feb 11, 2019 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 2, 2019.
Scheduled code freeze: March 26, 2019

Generated by 🚫 dangerJS against e548997

@ryelle
Copy link
Member

ryelle commented Feb 12, 2019 •
edited
Loading

I saw this pop up in the #a11y channel and was reminded of this twitter thread, which basically says that when a link wraps an image, the alt text should refer to the link's destination. Otherwise the screen reader will read something like "Link: A sunset on the beach", rather than "Link: My trip to the coast" – the post title makes more sense as the navigation target.

Other ideas:

  • We could remove the link from the image, and only have it on the post title, preventing the duplicate link.
  • We hide the image from the screen reader/keyboard user by adding aria-hidden and tabindex="-1" like in Twenty Nineteen

Does this only affect the output of the block? I don't have a jetpack site to test with ATM but would be happy to set it up & try it with VoiceOver if you can give a more detailed test flow 🙂

Edit: as soon as I posted I saw this was "in progress", sorry for the premature feedback! 😨 my offer to test still stands when this is ready.

@aldavigdis
Copy link
Contributor Author

@ryelle I'd be happy to remove the link from the image. However, I generally don't want to hide content from assistive technologies, including screenreaders as it generally gets very negative feedback.

@ryelle
Copy link
Member

ryelle commented Feb 12, 2019

Of course, having the image + alt without the link is a better option, but sometimes you want the full click-target of the image, so that was an alternate solution.

@aldavigdis aldavigdis force-pushed the related-posts-improve-markup-and-add-alt-text branch 2 times, most recently from 8a74d90 to ff9fed2 Compare February 12, 2019 12:57
@aldavigdis
Copy link
Contributor Author

@ryelle This is a WIP, but is intended to nail down the markup, so the sooner I get a11y feedback on this, the better. :)

@aldavigdis aldavigdis force-pushed the related-posts-improve-markup-and-add-alt-text branch from ff9fed2 to 78f4157 Compare February 13, 2019 14:59
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello aldavigdis! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D24307-code before merging this PR. Thank you!

@aldavigdis aldavigdis force-pushed the related-posts-improve-markup-and-add-alt-text branch from 78f4157 to 1aa1d36 Compare February 13, 2019 16:12
@matticbot
Copy link
Contributor

aldavigdis, Your synced wpcom patch D24307-code has been updated.

@aldavigdis
Copy link
Contributor Author

I have cherry-picked some of the commits here into separate PRs to make review and communication easier:

I'd like those to be merged first and then this one can be rebased and merged.

@ryelle
Copy link
Member

ryelle commented Feb 13, 2019

@aldavigdis I know you want feedback sooner, but I'm unclear about whether this is functionally ready to be tested, and how to test it. How can I see related posts on a test site?

@aldavigdis aldavigdis self-assigned this Feb 13, 2019
@aldavigdis aldavigdis mentioned this pull request Feb 15, 2019
Closed
@aldavigdis aldavigdis force-pushed the related-posts-improve-markup-and-add-alt-text branch from 1aa1d36 to 4e35dda Compare February 15, 2019 12:30
@matticbot
Copy link
Contributor

aldavigdis, Your synced wpcom patch D24307-code has been updated.

@aldavigdis aldavigdis force-pushed the related-posts-improve-markup-and-add-alt-text branch from 4e35dda to 5287292 Compare February 26, 2019 13:11
@matticbot
Copy link
Contributor

aldavigdis, Your synced wpcom patch D24307-code has been updated.

@aldavigdis aldavigdis mentioned this pull request Feb 26, 2019
Closed
@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Feb 26, 2019
@jeherve jeherve modified the milestones: 7.1, 7.2 Feb 26, 2019
@aldavigdis
Copy link
Contributor Author

I can't seem to reproduce those errors and notices.

@aldavigdis aldavigdis force-pushed the related-posts-improve-markup-and-add-alt-text branch from 4037b19 to 5f53959 Compare February 26, 2019 18:11
@matticbot
Copy link
Contributor

aldavigdis, Your synced wpcom patch D24307-code has been updated.

Redoing HTML markup for Related Posts (again)
1c9bdb2 
- Using alt text for images instead of a redundant article title
- Using sprintf to output HTML markup
- Properly using nav and ul elements to indicate related posts
@aldavigdis aldavigdis force-pushed the related-posts-improve-markup-and-add-alt-text branch from 5f53959 to 1c9bdb2 Compare February 26, 2019 18:17
@matticbot
Copy link
Contributor

aldavigdis, Your synced wpcom patch D24307-code has been updated.

jeherve
jeherve requested changes Mar 22, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works well for me now. I only have a minor remark.

@jeherve @kraftbj
Update modules/related-posts/jetpack-related-posts.php
e548997 
Co-Authored-By: kraftbj <public@brandonkraft.com>
@matticbot
Copy link
Contributor

aldavigdis, Your synced wpcom patch D24307-code has been updated.

@kraftbj kraftbj added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 25, 2019
jeherve
jeherve approved these changes Mar 26, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well for me now. 👍

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 26, 2019
@kraftbj kraftbj merged commit 1320ca5 into master Mar 26, 2019
@ghost ghost removed [Status] Needs Changelog [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 26, 2019
@kraftbj kraftbj deleted the related-posts-improve-markup-and-add-alt-text branch March 26, 2019 15:00
kraftbj added a commit that referenced this pull request Mar 26, 2019
@kraftbj
CL #11324
a2ff6c5
kraftbj added a commit that referenced this pull request Mar 27, 2019
@kraftbj
Add initial 7.2 changelog to master (#11690)
2227bfd 
* Initial Changelog for 7.2

* Testing list: add mention of IE11 testing

* Initial Changelog for 7.2

* Testing list: add mention of IE11 testing

* Add CL for #11224

* Add CL for #11426

* Add CL for #11442

* Add testing instructions for #11224

* Add CL for #11451

* Reclassify CL item

* Add testing instructions for #11451

* Add CL for #11486

* Add CL for #11418

* Add CL for #11524

* Add CL and testing instructions for #11449

* Add CL for #11460

* Add CL for #11520 and #11582

* Add CL for #11531

* Add CL #11644

* Add testing instructions for #11644

* Add testing instructions for #11644

* Add CL for #11618

* Uniform changelog lines

* CL #11679

* CL #11661

* CL #11654

* CL #11645

* CL #11643

* CL #11636

* CL #11635 and for other PHPCS commits

* CL #11627

* CL #11626

* CL #11598

* CL #11596

* Remove nested items for shortcopy. I don't believe the detailed list is helpful

* CL #11570

* CL #11569

* CL #11560

* CL #11558

* CL #11555

* CL #6704

* CL #11298

* CL #11324

* CL #11443

* CL #11484

* CL #11516

* CL #11529

* Expand Ads block enhancement CL item
@jeherve jeherve mentioned this pull request Apr 11, 2019
Merged
jeherve added a commit that referenced this pull request Apr 11, 2019
@jeherve
Related Posts: do not use nested rules in vanilla CSS file
4448150 
In #11324 we made some changes to the Related Posts layout, and introduced nested rules for
CSS. Since that is not supported in vanilla CSS, let's switch to regular CSS here.
jeherve added a commit that referenced this pull request Apr 11, 2019
@jeherve
Related Posts: do not use nested rules in vanilla CSS file (#12020)
9e361f8 
In #11324 we made some changes to the Related Posts layout, and introduced nested rules for
CSS. Since that is not supported in vanilla CSS, let's switch to regular CSS here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve approved these changes

@lezama lezama Awaiting requested review from lezama

Assignees

@aldavigdis aldavigdis

Labels
[Feature] Related Posts [Focus] Accessibility Improving usability for all users (a11y) [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.
7 participants
@aldavigdis @jetpackbot @ryelle @matticbot @roccotripaldi @jeherve @kraftbj