Make WordPress Core

Opened 7 years ago

Closed 6 years ago

#43632 closed enhancement (fixed)

Add a screen reader text for Hello Dolly lyrics

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: audrasjb's profile audrasjb
Milestone: 5.2 Priority: normal
Severity: normal Version:
Component: Plugins Keywords: has-screenshots has-patch
Focuses: accessibility Cc:

Description

Background: #43555

Originally suggested by @TJNowell in comment:30:ticket:43555:

Would it make sense to add a musical note symbol to clarify that it is indeed lyrics? Perhaps turn it into a link to the full Hello Dolly lyrics so it has some context?

Even without a link, a note symbol seems like an easy improvement adding some sort of context. See the screenshot with beamed eighth notes (♫).

Attachments (16)

43632.PNG (1.8 KB) - added by SergeyBiryukov 7 years ago.
43632diff-bad-behaviour.png (38.8 KB) - added by audrasjb 7 years ago.
In 43632.diff, I noticed there is a missing padding left (or right, depends on rtl/ltr) on widget admin screen
43632diff-nice-behaviour.png (32.3 KB) - added by audrasjb 7 years ago.
43632.diff full correction including paddings
43632.diff (833 bytes) - added by audrasjb 7 years ago.
Add a musical note, screen-reader-text and some padding to the musical quote from Hello Dolly
plugin-off.png (140.6 KB) - added by desrosj 7 years ago.
plugin-on-cramped.png (150.6 KB) - added by desrosj 7 years ago.
43632.2.diff (1007 bytes) - added by audrasjb 7 years ago.
43632.2.diff - CSS update
43632-dolly-height.png (40.7 KB) - added by audrasjb 7 years ago.
The total height is now the same that screen options button
43632-responsive-height.png (31.5 KB) - added by audrasjb 7 years ago.
Consistent height in mobile view
43632-responsive-layout.gif (3.5 MB) - added by audrasjb 7 years ago.
Responsive behaviour
43632.3.diff (1.2 KB) - added by audrasjb 7 years ago.
In 43632.3.diff: replace unnecessary is_rtl() conditional tag with body_class CSS rule
43632.4.diff (1.3 KB) - added by audrasjb 6 years ago.
Screen reader text + lang attribute if needed
43632.5.diff (1.6 KB) - added by audrasjb 6 years ago.
Previous changes + translatable string
43632.6.diff (1.6 KB) - added by audrasjb 6 years ago.
43632.7.diff (1.7 KB) - added by audrasjb 6 years ago.
Refresh + move lang attribute to <p> tag
43632.8.diff (3.4 KB) - added by afercia 6 years ago.

Change History (64)

@SergeyBiryukov
7 years ago

#1 @danieltj
7 years ago

  • Focuses accessibility added
  • Keywords needs-patch added

Thinking with accessibility in mind, would we want to have a screen reader read aloud the note or not? I'm thinking it's probably important to denote that it a piece of song lyrics so when read out it's not confusing it switches from settings to a song.

#2 @afercia
7 years ago

Adding some visually hidden text would be helpful, as right now there's really nothing in the markup to clarify what the Hello Dolly text is:

https://cldup.com/4OC7r8p3Cw.png

#3 follow-up: @birgire
7 years ago

There's also an existing dashicon for:

<span class="dashicons dashicons-format-audio"></span>

https://developer.wordpress.org/resource/dashicons/#format-audio

but that might already be too integrated with audios in wp-admin so the suggested &#9835; would probably be better.

#4 in reply to: ↑ 3 @SergeyBiryukov
7 years ago

Replying to birgire:

There's also an existing dashicon

Yeah, I considered using Dashicons, but they're only available in WordPress 3.8+, and I think there's a non-zero chance of someone updating the plugin on 3.7 or earlier (as unlikely as it may be), so using a Unicode symbol seems safer.

#5 follow-up: @karmatosed
7 years ago

With the changes in lyrics is this still needed? I wonder if we're adding confusion by adding a musical note? For example, would a user be even more puzzled? Would they think they should click to hear? I am not sure it helps in this instance.

#6 @birgire
7 years ago

Additionally I wonder if the line should be displayed with quotation marks?

But in that case I think we would need to translate the quotation marks, as they can vary in different languages. Hello Dolly is not using any translations, so that is currently not an option. Maybe there's an existing core string for quotations marks? But then again I don't think it's recommended to re-use translated core strings in plugins, as they can always change.

Alternatively there's also the quote dashicon:

<span class="dashicons dashicons-format-quote"></span>

but using that icon in front might look strange to some.

#7 in reply to: ↑ 5 @SergeyBiryukov
7 years ago

Replying to karmatosed:

With the changes in lyrics is this still needed? I wonder if we're adding confusion by adding a musical note?

I think at least a screen reader comment for assistive technologies would be nice, as noted in comment:2.

For example, would a user be even more puzzled? Would they think they should click to hear?

If it doesn't look a link, I wouldn't expect anyone to click on it :)

@audrasjb
7 years ago

In 43632.diff, I noticed there is a missing padding left (or right, depends on rtl/ltr) on widget admin screen

@audrasjb
7 years ago

43632.diff full correction including paddings

@audrasjb
7 years ago

Add a musical note, screen-reader-text and some padding to the musical quote from Hello Dolly

#8 @audrasjb
7 years ago

  • Keywords has-patch ui-feedback added; 2nd-opinion needs-patch removed

Hi,

In 43632.diff:

  • Add a musical note ASCII character
  • Add a screen-reader-text to help to understand what is this text
  • Add some paddings to the musical quote from Hello Dolly (see screenshots above)

Cheers,
Jb

@desrosj
7 years ago

#9 @desrosj
7 years ago

I tried testing this with VoiceOver in Chrome, but I was unable to use the keyboard to actually navigate to the lyrics. I am a novice when it comes to testing with VoiceOver, though, so I could be missing something.

That aside, the patch looks good, but I have a few things that I noticed.

  • On mobile, the lyric gets a little cramped (plugin-on-cramped.png). When the plugin is off, the Add New button is pulled up and it feels fine (plugin-off.png), but I find myself wanting the height of the lyric to be similar to the Screen Options height, or at least have some bottom padding.
  • The baseline for the lyric is also slightly off from the screen options tab and feels off to me.

@audrasjb
7 years ago

43632.2.diff - CSS update

@audrasjb
7 years ago

The total height is now the same that screen options button

@audrasjb
7 years ago

Consistent height in mobile view

@audrasjb
7 years ago

Responsive behaviour

#10 @audrasjb
7 years ago

Hi @desrosj and thanks for the review,

In 43632.diff :

  • Same height for both #dolly and "Screen options" button (see screenshots)
  • Fix "Hello Dolly" line height to be consistent with Screen options buttons
  • Adds media queries for mobile view (see animated screenshot)
  • Adds RTL CSS rule to support RTL languages (padding changes)

Note: indeed, you can't access Hello Dolly lyrics with TAB navigation on voice hover because you simply navigate through links. But I think this is nice: we don't need any anchor here.

Cheers,
Jb

#11 @audrasjb
7 years ago

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

#12 @audrasjb
7 years ago

  • Keywords ui-feedback removed

@audrasjb
7 years ago

In 43632.3.diff: replace unnecessary is_rtl() conditional tag with body_class CSS rule

This ticket was mentioned in Slack in #core by desrosj. View the logs.


7 years ago

#14 @desrosj
7 years ago

  • Milestone changed from 4.9.6 to 4.9.7

#15 @desrosj
7 years ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

This ticket was mentioned in Slack in #core by joshuawold. View the logs.


6 years ago

#17 @afercia
6 years ago

I tried testing this with VoiceOver in Chrome, but I was unable to use the keyboard to actually navigate to the lyrics.

@desrosj to navigate trough all elements use Ctrl-Option-Right arrow (or Left arrow to go back). Your best option would be testing with VoiceOver and Safari though, not Chrome. Instead, Windows screen readers use the Up and Down arrows.

With regards to the patch, just a small thing: adding a colon : at the end of the screen reader text would help, making screen readers do a brief pause. Currently, the string is announced all together without any pause:

http://cldup.com/JXGoaohrgm.png

Last edited 6 years ago by afercia (previous) (diff)

#18 @karmatosed
6 years ago

I have to say I still feel the note is not needed here. I totally am ok with adding something in for screen reader text but the icon just feels out of place and like it's trying to be an interaction.

#19 @SergeyBiryukov
6 years ago

  • Keywords needs-refresh added
  • Summary changed from Add a musical note symbol to Hello Dolly lyrics to Add a screen reader text for Hello Dolly lyrics

Let's add a screen reader text without the note symbol then.

#20 @audrasjb
6 years ago

  • Keywords needs-refresh removed

Hi,

In 43632.4.diff:

  1. Remove the musical note symbol
  2. Add screen reader text with colon : at the end of the text
  3. Position the text correctly in responsive and RTL languages
  4. Add lang="en" attribute only if needed (when get_user_locale() not equal to en_*) for better vocalisation of english text since Hello Dolly plugin is not translatable

@afercia can you please review item 4 of that "changelog"?
Thanks ;-)

Cheers,

Jb

Last edited 6 years ago by audrasjb (previous) (diff)

@audrasjb
6 years ago

Screen reader text + lang attribute if needed

#21 @afercia
6 years ago

@audrasjb the lang attribute is a nice touch :) However, I think the screen reader text "Quote from Hello Dolly song:" should be translatable and the lang attribute applied only to the Hello Dolly text.

#22 @audrasjb
6 years ago

@afercia thanks for the review ;-)

However, Hello Dolly won't be translatable as far as I know, and that's why I added lang attribute to the whole parapgraph.

#23 @afercia
6 years ago

Sure, the Hello Dolly lyrics must stay in English. However, when switching to my language, I'd like to hear "Quote from Hello Dolly song:" in my language ;)

#24 @pento
6 years ago

4.9.8 has hit RC, moving to 4.9.9.

#25 @pento
6 years ago

  • Milestone changed from 4.9.8 to 4.9.9

#26 @audrasjb
6 years ago

@afercia this is not my point

As far as I know, Hello Dolly plugin is not translatable at all, so we don't need to make this string translatable since it has no effect :)

#27 follow-up: @afercia
6 years ago

@audrasjb not sure I understand :) I see Hello Dolly is on Glotpress (e.g. for Italian): https://translate.wordpress.org/locale/it/default/wp-plugins/hello-dolly

#28 in reply to: ↑ 27 @audrasjb
6 years ago

@afercia Sooorry. Hello Dolly is now correctly prepared for translation! That was not the case one year before.

I'll update the patch then :p

#29 follow-up: @afercia
6 years ago

On a side note: does this mean the info in the plugin header (e.g. Author and Author URI) are actually translatable? Seems it is possible to "translate" Matt Mullenweg and his blog URL (and I guess this applies to all plugins). Doesn't look so right to me, but maybe I'm missing something :) /Cc @pento

http://cldup.com/XX7AIYBf6Y.png

#30 in reply to: ↑ 29 @audrasjb
6 years ago

Yep, and same for every plugins on WordPress.org, but since every translation changes need validation from GTE/GPTE/PTE, I think it's safe :)

Last edited 6 years ago by audrasjb (previous) (diff)

@audrasjb
6 years ago

Previous changes + translatable string

#31 follow-up: @birgire
6 years ago

@audrasjb Thanks for the patch in 43632.5.diff

Few suggestions:

I was wondering if we could move the extra space from the HTML tag, into the $lang variable?

Also if we should use substr instead of strpos, since we are targeting the first three (non multibyte) chars?

Also if we could avoid a very long echo line.

So instead of:

$lang = "";
if ( false === strpos( get_user_locale(), 'en_' ) ) {
	$lang = "lang='en'";
}
echo "<p id='dolly' $lang><span class='screen-reader-text'>" . __( 'Quote from Hello Dolly song:', 'hello-dolly' ) . " </span>$chosen</p>";

we might have:

$lang = '';
if ( 'en_' === substr( get_user_locale(), 0, 3 ) ) {
	$lang = " lang='en'";
}

printf(
	"<p id='dolly'%s><span class='screen-reader-text'>%s </span>%s</p>",
	$lang,
	__( 'Quote from Hello Dolly song:', 'hello-dolly' ),
	$chosen
);

I was also curious what are the available locales that start with en_.

I could find these: en_US, en_AU, en_CA, en_NZ, en_ZA, en_GB on https://translate.wordpress.org.

There seems to be English (Pirate) available too with art_xpirate ?

See more about it here:

https://translate.wordpress.org/locale/pirate

https://make.wordpress.org/polyglots/2017/01/05/hi-4/

Should it be included here?

Last edited 6 years ago by birgire (previous) (diff)

#32 in reply to: ↑ 31 @audrasjb
6 years ago

Replying to birgire:

I was wondering if we could move the extra space from the HTML tag, into the $lang variable?

Sure, but I think we should also move it to a span element since the paragraphe is translatable now… :D

Also if we should use substr instead of strpos, since we are targeting the first three (non multibyte) chars?
Also if we could avoid a very long echo line.
So instead of:
[…]
we might have:
[…]

You are totally right!

There seems to be English (Pirate) available too with art_xpirate ?
Should it be included here?

Good catch!

Do you want to add a new patch? I can make it but maybe you are about to do so… :)

Thanks,

Jb

#33 @birgire
6 years ago

@audrasjb thanks for the quick reply, feel free to update the patch, I'm going offline for few hours :-)

@audrasjb
6 years ago

#34 @audrasjb
6 years ago

In 43632.6.diff after @birgire comments:

Here is the generated HTML source:

<p id="dolly">
    <span class="screen-reader-text">Quote from Hello Dolly song: </span>
    <span>Well, hello, Dolly</span>
</p>

I have not added Pirate locale since it is pretty different than classic english (there is a lot of words from other languages).

Cheers,

Jb

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


6 years ago

#36 @pento
6 years ago

  • Milestone changed from 4.9.9 to Future Release

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


6 years ago

#38 @afercia
6 years ago

  • Milestone changed from Future Release to 5.2

Moving to 5.2 consideration.

#39 @afercia
6 years ago

  • Keywords needs-refresh added

The patch doesn't apply cleanly and needs a refresh.

Also, I think the span element wrapping the lyrics needs a lang="en-US" attribute, e.g.:

`<span lang="en-US">Well, hello, Dolly</span>`

When switching the admin to another language, e.g. French, I guess users would still appreciate to hear the lyrics with the correct screen reader voice and accent.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


6 years ago

#41 @afercia
6 years ago

Note from today's accessibility bug-scrub: consider to make the screen reader text say:
"Quote from "Hello, Dolly", by Jerry Herman"

@audrasjb
6 years ago

Refresh + move lang attribute to <p> tag

#42 @audrasjb
6 years ago

  • Keywords needs-refresh removed

In 43632.7.diff :

  • Patch refreshed
  • Fix a comparison operator bug introduced in previous patches
  • Add author credits on the screen reader text
  • Move lang attribute to the <p> tag

Here is the rendering for english installs:

<p id="dolly">
	<span class="screen-reader-text">Quote from Hello Dolly song, by Jerry Herman: </span>
	<span>Well, hello, Dolly</span>
</p>

And for non english installs:

<p id="dolly" lang="en">
	<span class="screen-reader-text">Quote from Hello Dolly song, by Jerry Herman: </span>
	<span>You’re still goin’ strong</span>
</p>

Tested and works fine on my side :-)

Last edited 6 years ago by audrasjb (previous) (diff)

#43 @audrasjb
6 years ago

  • Keywords commit added

#44 @afercia
6 years ago

Noticed the current translated strings are in the admin-xx_XX.po file. Can someone confirm the Hello Dolly strings are considered "core strings" and thus we shouldn't add Text Domain: hello-dolly? Thanks.
/Cc @SergeyBiryukov @ocean90

@afercia
6 years ago

#45 @afercia
6 years ago

  • Keywords commit removed

43632.8.diff updates a few things, more importantly:

  • removes the text domain for now (pending confirmation)
  • updates the plugin version, also in a related test
  • moves lang="en" to the lyrics because the part Quote from Hello Dolly song, by Jerry Herman: should be translated and pronounced in the translated language. Example (emulating translation in Italian):

http://cldup.com/ZravpoH65R.png

  • makes the font size a bit bigger: 11px was really too small
  • simplifies the CSS so that only float needs to be reverted for RTL languages
  • adds a dir="ltr" to the lyrics: needed when in RTL especially for strings that end with an apostrophe
  • improves RTL on mobile

Note: ideally, in the text:
Quote from Hello Dolly song, by Jerry Herman:
the parts Hello Dolly and Jerry Herman would need a lang="en" attribute because the song title and author name should be pronounced in English. However, for the sake of simplicity and to not add to many lang attributes, I'd lean towards keeping it as is.

This ticket was mentioned in Slack in #core by afercia. View the logs.


6 years ago

#47 @afercia
6 years ago

For history: clarified the text domain thing after conversation on Slack. The Hello Dolly version shipped with core doesn't need a text domain: its strings are considered part of core. The standalone version on the plugin repo does use a text domain and it gets synced from time to time with the core version.

#48 @afercia
6 years ago

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

In 44929:

Accessibility: Improve the "Hello Dolly" accessibility.

  • adds a visually hidden text to give context to the lyrics
  • the text is Quote from Hello Dolly song, by Jerry Herman:
  • adds a lang HTML attribute (when the admin language is not English) to better support assistive technologies
  • adds a dir HTML attribute to better support the LTR English lyrics with RTL languages
  • CSS adjustments

Props audrasjb, SergeyBiryukov, danieltj, birgire, karmatosed, desrosj, afercia.
Fixes #43632.

Note: See TracTickets for help on using tickets.