WordPress.org

Make WordPress Core

Opened 17 months ago

Closed 5 months ago

#43632 closed enhancement (fixed)

Add a screen reader text for Hello Dolly lyrics

Reported by: SergeyBiryukov Owned by: 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 17 months ago.
43632diff-bad-behaviour.png (38.8 KB) - added by audrasjb 17 months 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 17 months ago.
43632.diff full correction including paddings
43632.diff (833 bytes) - added by audrasjb 17 months 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 16 months ago.
plugin-on-cramped.png (150.6 KB) - added by desrosj 16 months ago.
43632.2.diff (1007 bytes) - added by audrasjb 16 months ago.
43632.2.diff - CSS update
43632-dolly-height.png (40.7 KB) - added by audrasjb 16 months ago.
The total height is now the same that screen options button
43632-responsive-height.png (31.5 KB) - added by audrasjb 16 months ago.
Consistent height in mobile view
43632-responsive-layout.gif (3.5 MB) - added by audrasjb 16 months ago.
Responsive behaviour
43632.3.diff (1.2 KB) - added by audrasjb 16 months 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 13 months ago.
Screen reader text + lang attribute if needed
43632.5.diff (1.6 KB) - added by audrasjb 11 months ago.
Previous changes + translatable string
43632.6.diff (1.6 KB) - added by audrasjb 11 months ago.
43632.7.diff (1.7 KB) - added by audrasjb 5 months ago.
Refresh + move lang attribute to <p> tag
43632.8.diff (3.4 KB) - added by afercia 5 months ago.

Change History (64)

#1 @danieltj
17 months 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
17 months 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
17 months 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
17 months 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
17 months 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
17 months 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
17 months 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
17 months ago

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

@audrasjb
17 months ago

43632.diff full correction including paddings

@audrasjb
17 months ago

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

#8 @audrasjb
17 months 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
16 months ago

#9 @desrosj
16 months 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
16 months ago

43632.2.diff - CSS update

@audrasjb
16 months ago

The total height is now the same that screen options button

@audrasjb
16 months ago

Consistent height in mobile view

@audrasjb
16 months ago

Responsive behaviour

#10 @audrasjb
16 months 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
16 months ago

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

#12 @audrasjb
16 months ago

  • Keywords ui-feedback removed

@audrasjb
16 months 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.


16 months ago

#14 @desrosj
16 months ago

  • Milestone changed from 4.9.6 to 4.9.7

#15 @desrosj
15 months 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.


14 months ago

#17 @afercia
14 months 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:

https://cldup.com/JXGoaohrgm.png

Version 0, edited 14 months ago by afercia (next)

#18 @karmatosed
14 months 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
13 months 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
13 months 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 13 months ago by audrasjb (previous) (diff)

@audrasjb
13 months ago

Screen reader text + lang attribute if needed

#21 @afercia
13 months 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
13 months 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
13 months 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
13 months ago

4.9.8 has hit RC, moving to 4.9.9.

#25 @pento
13 months ago

  • Milestone changed from 4.9.8 to 4.9.9

#26 @audrasjb
11 months 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
11 months 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
11 months 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
11 months 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
11 months 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 11 months ago by audrasjb (previous) (diff)

@audrasjb
11 months ago

Previous changes + translatable string

#31 follow-up: @birgire
11 months 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 11 months ago by birgire (previous) (diff)

#32 in reply to: ↑ 31 @audrasjb
11 months 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
11 months ago

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

@audrasjb
11 months ago

#34 @audrasjb
11 months 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.


11 months ago

#36 @pento
11 months ago

  • Milestone changed from 4.9.9 to Future Release

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


7 months ago

#38 @afercia
7 months ago

  • Milestone changed from Future Release to 5.2

Moving to 5.2 consideration.

#39 @afercia
6 months 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.


5 months ago

#41 @afercia
5 months ago

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

@audrasjb
5 months ago

Refresh + move lang attribute to <p> tag

#42 @audrasjb
5 months 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 5 months ago by audrasjb (previous) (diff)

#43 @audrasjb
5 months ago

  • Keywords commit added

#44 @afercia
5 months 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
5 months ago

#45 @afercia
5 months 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.


5 months ago

#47 @afercia
5 months 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
5 months 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.