Opened 7 years ago
Closed 6 years 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)
Change History (64)
#3
follow-up:
↓ 4
@
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 ♫
would probably be better.
#4
in reply to:
↑ 3
@
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:
↓ 7
@
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
@
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
@
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 :)
@
7 years ago
In 43632.diff, I noticed there is a missing padding left (or right, depends on rtl/ltr) on widget admin screen
@
7 years ago
Add a musical note, screen-reader-text and some padding to the musical quote from Hello Dolly
#8
@
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
#9
@
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.
#10
@
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
@
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
This ticket was mentioned in Slack in #core by joshuawold. View the logs.
6 years ago
#17
@
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:
#18
@
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
@
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
@
6 years ago
- Keywords needs-refresh removed
Hi,
In 43632.4.diff
:
- Remove the musical note symbol
- Add screen reader text with colon
:
at the end of the text - Position the text correctly in responsive and RTL languages
- Add
lang="en"
attribute only if needed (whenget_user_locale()
not equal toen_*
) 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
#21
@
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
@
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
@
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 ;)
#26
@
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:
↓ 28
@
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
@
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:
↓ 30
@
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
#30
in reply to:
↑ 29
@
6 years ago
Yep, and same for every plugins on WordPress.org, but since every translations need a validation, I think it's safe :)
#31
follow-up:
↓ 32
@
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?
#32
in reply to:
↑ 31
@
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
@
6 years ago
@audrasjb thanks for the quick reply, feel free to update the patch, I'm going offline for few hours :-)
#34
@
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
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
6 years ago
#39
@
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
@
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"
#42
@
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 :-)
#44
@
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
#45
@
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 partQuote 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.
This ticket was mentioned in Slack in #core by afercia. View the logs.
6 years ago
#47
@
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.
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.