Make WordPress Core

Opened 6 years ago

Closed 3 months ago

Last modified 3 months ago

#46233 closed defect (bug) (fixed)

Multiple themes: Theme includes Screen Reader Text inside Card

Reported by: dshanske's profile dshanske Owned by: karmatosed's profile karmatosed
Milestone: 6.7 Priority: low
Severity: trivial Version:
Component: Bundled Theme Keywords: has-patch commit
Focuses: Cc:

Description (last modified by karmatosed)

The function twentysixteen_entry_meta includes the screen reader text inside the span property using the author mf class. That means when parsed it would say, 'Author David Shanske' The screen reader text should be inside the byline span but outside the author vcard span.

Added: this is in both Twenty Sixteen and Twenty Fifteen

Attachments (3)

46233.patch (1.8 KB) - added by sabernhardt 3 years ago.
46233.2.patch (2.0 KB) - added by shailu25 3 months ago.
Patch Refreshed
46233.3.patch (1.8 KB) - added by sabernhardt 3 months ago.
removes space between avatar and screen reader text

Download all attachments as: .zip

Change History (19)

#1 @laurelfulford
6 years ago

  • Component changed from Themes to Bundled Theme
  • Summary changed from Twenty Sixteen Theme includes Screen Reader Text inside Card to Twenty Sixteen: Theme includes Screen Reader Text inside Card

#2 @laurelfulford
6 years ago

  • Keywords reporter-feedback added

Thanks for this report, @dshanske!

I see what you mean. Unfortunately, this could be tricky to change without causing other issues -- moving <span class="screen-reader-text">Author </span> outside of the .author span would also place the author image between that string and the actual author name, making it less meaningful to screen readers.

I'm far from an expert on Microformats, but in this case, would adding role to that screen-reader-text be "correct"? Or am I totally reaching? :)

@sabernhardt
3 years ago

#3 @sabernhardt
3 years ago

  • Keywords has-patch added

With a (default) empty alt attribute, screen readers would ignore the avatar. However, the image could be moved before "Author" as well.

<span class="byline">
	<img alt="" src="..." class="avatar avatar-49 photo" loading="lazy" width="49" height="49">
	<span class="screen-reader-text">Author </span>
	<span class="author vcard">
		<a class="url fn n" href="http://localhost/svn/src/author/test/">test</a>
	</span>
</span>

Twenty Fifteen has a similar nesting, too.

#4 @karmatosed
6 months ago

  • Keywords reporter-feedback removed

#5 @karmatosed
3 months ago

  • Owner set to karmatosed
  • Status changed from new to assigned

Assigning to myself to see about testing towards a potential commit.

#6 @karmatosed
3 months ago

  • Description modified (diff)
  • Summary changed from Twenty Sixteen: Theme includes Screen Reader Text inside Card to Multiple themes: Theme includes Screen Reader Text inside Card

In testing I can confirm this issue is still valid and see that the patch does offer a solution which doesn't seem to impact the themes negatively. It is worth nothing this impacts both Twenty Sixteen and Twenty Fifteen so changing description to multiple themes.

#7 @karmatosed
3 months ago

  • Keywords commit added

#8 @karmatosed
3 months ago

  • Keywords needs-refresh added; commit removed

@sabernhardt I just went to apply this patch and it seems to get rejected when applying to Twenty Fifteen. I am going to put a refresh request on it but if this is an issue you can't replicate let me know please.

@shailu25
3 months ago

Patch Refreshed

#9 @shailu25
3 months ago

  • Keywords needs-refresh removed

#10 @sabernhardt
3 months ago

Thanks for refreshing the patch! Reviewing that, I found an extra space I should not have added after %1$s in Twenty Sixteen.

  • That space is simply unnecessary following the avatar:
    <span class="byline"><img alt='' src='http://0.gravatar.com/avatar/0309805bf7f3bc7b9a30f6e5c772475a?s=49&#038;d=mm&#038;r=g' srcset='http://0.gravatar.com/avatar/0309805bf7f3bc7b9a30f6e5c772475a?s=98&#038;d=mm&#038;r=g 2x' class='avatar avatar-49 photo' height='49' width='49' decoding='async'/> <span class="screen-reader-text">Author </span><span class="author vcard"><a class="url fn n" href="http://localhost/svn/src/author/test/">tester</a></span></span>
  • It makes the markup look wrong without avatars enabled:
    <span class="byline"> <span class="screen-reader-text">Author </span><span class="author vcard"><a class="url fn n" href="http://localhost/svn/src/author/test/">tester</a></span></span>

@sabernhardt
3 months ago

removes space between avatar and screen reader text

#11 @sabernhardt
3 months ago

  • Milestone changed from Awaiting Review to Future Release

Twenty Fifteen

  • Before patch:
    <span class="byline"><span class="author vcard"><span class="screen-reader-text">Author </span><a class="url fn n" href="http://localhost/svn/src/author/test/">tester</a></span></span>
  • With 46233.3.patch:
    <span class="byline"><span class="screen-reader-text">Author </span><span class="author vcard"><a class="url fn n" href="http://localhost/svn/src/author/test/">tester</a></span></span>

Twenty Sixteen, with avatars enabled

  • Before patch:
    <span class="byline"><span class="author vcard"><img alt='' src='http://0.gravatar.com/avatar/0309805bf7f3bc7b9a30f6e5c772475a?s=49&#038;d=mm&#038;r=g' srcset='http://0.gravatar.com/avatar/0309805bf7f3bc7b9a30f6e5c772475a?s=98&#038;d=mm&#038;r=g 2x' class='avatar avatar-49 photo' height='49' width='49' decoding='async'/><span class="screen-reader-text">Author </span> <a class="url fn n" href="http://localhost/svn/src/author/test/">tester</a></span></span>
  • With 46233.3.patch:
    <span class="byline"><img alt='' src='http://0.gravatar.com/avatar/0309805bf7f3bc7b9a30f6e5c772475a?s=49&#038;d=mm&#038;r=g' srcset='http://0.gravatar.com/avatar/0309805bf7f3bc7b9a30f6e5c772475a?s=98&#038;d=mm&#038;r=g 2x' class='avatar avatar-49 photo' height='49' width='49' decoding='async'/><span class="screen-reader-text">Author </span><span class="author vcard"><a class="url fn n" href="http://localhost/svn/src/author/test/">tester</a></span></span>

Twenty Sixteen, without avatar

  • Before patch:
    <span class="byline"><span class="author vcard"><span class="screen-reader-text">Author </span><a class="url fn n" href="http://localhost/svn/src/author/test/">tester</a></span></span>
  • With 46233.3.patch:
    <span class="byline"><span class="screen-reader-text">Author </span><span class="author vcard"><a class="url fn n" href="http://localhost/svn/src/author/test/">tester</a></span></span>

#12 @karmatosed
3 months ago

  • Keywords commit added

Thank you for the patch refresh and extensive testing. I am now going to look at getting this to commit.

#13 @karmatosed
3 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 58771:

Multiple themes Fixes theme screen reader including text inside card.

The function twentysixteen_entry_meta included screen reader text inside the span property using the author mf class. This resolves that in both Twenty Sixteen and Twenty Fifteen. It should have the screen reader text inside the byline span but outside the author vcard span.

Props dshanske, laurelfulford, sabernhardt, shilu25.
Fixes #46233.

#14 @shailu25
3 months ago

@karmatosed My Username is wrong which you have mentioned in prop list. It should be shailu25 instead of shilu25.

Last edited 3 months ago by shailu25 (previous) (diff)

#15 @karmatosed
3 months ago

@shailu25 I apologise; I can fix that.

#16 @desrosj
3 months ago

  • Milestone changed from Future Release to 6.7
Note: See TracTickets for help on using tickets.