Add a screen reader text for Hello Dolly lyrics

[SergeyBiryukov]

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 (♫).

[danieltj]

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.

[afercia]

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:

[birgire]

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 ♫ would probably be better.

[SergeyBiryukov]

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.

[karmatosed]

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.

[birgire]

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.

[SergeyBiryukov]

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]

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

[audrasjb]

43632.diff full correction including paddings

[audrasjb]

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

[audrasjb]

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]

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]

43632.2.diff - CSS update

[audrasjb]

The total height is now the same that screen options button

[audrasjb]

Consistent height in mobile view

[audrasjb]

Responsive behaviour

[audrasjb]

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

[audrasjb]

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

[desrosj]

Moving all tickets in 4.9.7 to 4.9.8.

[afercia]

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:

[karmatosed]

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.

[SergeyBiryukov]

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

[audrasjb]

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

[audrasjb]

Screen reader text + lang attribute if needed

[afercia]

@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.

[audrasjb]

@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.

[afercia]

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 ;)

[pento]

4.9.8 has hit RC, moving to 4.9.9.

[audrasjb]

@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 :)

[afercia]

@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

[audrasjb]

@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

[afercia]

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

[audrasjb]

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 :)

[audrasjb]

Previous changes + translatable string

[birgire]

@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?

[audrasjb]

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

[birgire]

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

[audrasjb]

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

[afercia]

Moving to 5.2 consideration.

[afercia]

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.

[afercia]

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

[audrasjb]

Refresh + move lang attribute to <p> tag

[audrasjb]

In 43632.7.diff :

• Patch refreshed
• 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 :-)

[afercia]

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]

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):

• 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.

[afercia]

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.

[afercia]

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.