WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 4 weeks ago

#43632 assigned enhancement

Add a musical note symbol to Hello Dolly lyrics

Reported by: SergeyBiryukov Owned by: audrasjb
Milestone: 4.9.8 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 (11)

43632.PNG (1.8 KB) - added by SergeyBiryukov 3 months ago.
43632diff-bad-behaviour.png (38.8 KB) - added by audrasjb 3 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 3 months ago.
43632.diff full correction including paddings
43632.diff (833 bytes) - added by audrasjb 3 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 2 months ago.
plugin-on-cramped.png (150.6 KB) - added by desrosj 2 months ago.
43632.2.diff (1007 bytes) - added by audrasjb 2 months ago.
43632.2.diff - CSS update
43632-dolly-height.png (40.7 KB) - added by audrasjb 2 months ago.
The total height is now the same that screen options button
43632-responsive-height.png (31.5 KB) - added by audrasjb 2 months ago.
Consistent height in mobile view
43632-responsive-layout.gif (3.5 MB) - added by audrasjb 2 months ago.
Responsive behaviour
43632.3.diff (1.2 KB) - added by audrasjb 2 months ago.
In 43632.3.diff: replace unnecessary is_rtl() conditional tag with body_class CSS rule

Change History (26)

@SergeyBiryukov
3 months ago

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

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

@audrasjb
3 months ago

43632.diff full correction including paddings

@audrasjb
3 months ago

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

#8 @audrasjb
3 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
2 months ago

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

43632.2.diff - CSS update

@audrasjb
2 months ago

The total height is now the same that screen options button

@audrasjb
2 months ago

Consistent height in mobile view

@audrasjb
2 months ago

Responsive behaviour

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

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

#12 @audrasjb
2 months ago

  • Keywords ui-feedback removed

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


7 weeks ago

#14 @desrosj
7 weeks ago

  • Milestone changed from 4.9.6 to 4.9.7

#15 @desrosj
4 weeks ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

Note: See TracTickets for help on using tickets.