WordPress.org

Make WordPress Core

Opened 5 months ago

Last modified 4 weeks ago

#38932 new defect (bug)

Add unit (px) to suggested image size in customizer

Reported by: Presskopp Owned by:
Milestone: 4.8 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch
Focuses: accessibility Cc:

Description

When you go to Customizer > Header Media > Add new image

you see a notice about image size like:

Suggested image dimensions: 2000 × 1200

It should have a unit

Suggested image dimensions: 2000 × 1200 px

Attachments (5)

missing_px.jpg (37.7 KB) - added by Presskopp 5 months ago.
38932.patch (2.1 KB) - added by stormrockwell 5 months ago.
replaced 2000 x 1200 format to the confirmed 2000 by 1200 pixels format
38932.2.patch (2.8 KB) - added by stormrockwell 5 months ago.
Fixed hard-coded text
38932.3.patch (2.7 KB) - added by stormrockwell 5 months ago.
38932.4.patch (2.9 KB) - added by stormrockwell 5 months ago.
added translators comments

Download all attachments as: .zip

Change History (21)

@Presskopp
5 months ago

#1 @afercia
5 months ago

  • Focuses accessibility added
  • Version trunk deleted

From an accessibility perspective, worth noting all similar abbreviations should be avoided or expanded with the full form. For example, the suggested dimensions (including the missing "px") gets read out by a screen reader as:
one two zero zero times two eight zero pee ex
Numbers should be properly formatted too.
Recently, in another project, I've changed a similar string in "2000 by 1200 pixels" which is slightly better.

Same for the file size: one gee bee.

By the way, this applies to all the similar cases in core.

#2 @dd32
5 months ago

@afercia curiously, would <abbr title="pixels">px</abbr> work? (maybe with an aria-* attribute?)

#3 @afercia
5 months ago

@dd32 the title attribute works like a description and gets announced in addition to the visible text. So:

<abbr title="pixels">px</abbr> = pee ex (short pause here) pixels

Also, there's no guarantee the title attribute will be announced, it depends on the verbosity settings users can tweak in their screen readers.

The aria-label attribute replaces the content instead:

<span aria-label="pixels">px</span> = pixels

By the way, it tends to work best on interactive elements. Thinking also at the resulting translatable string, it would be simpler to use just plain text.

Last edited 5 months ago by afercia (previous) (diff)

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


5 months ago

#5 @rianrietveld
5 months ago

  • Milestone changed from Awaiting Review to Future Release

We discussed this in the accessibility bug scrub.
Thank you for this ticket @Presskopp

We concluded: just use plain simple 'pixels', adding extra HTML to explain px seems a unnecessary complication.
Also the x can be replaced by the word 'by' for a clear pronunciation by screen readers.
So the sentence will be:

2000 by 1200 pixels

Note by @celloexpressions that the custom header controls currently say 2000 &times; 1200 pixels, so that should be unified at some point.

@stormrockwell
5 months ago

replaced 2000 x 1200 format to the confirmed 2000 by 1200 pixels format

#6 @stormrockwell
5 months ago

  • Keywords has-patch added

#7 @westonruter
5 months ago

  • Keywords needs-patch added; has-patch removed

@stormrockwell thanks for the patch, but you have hard-coded English text in there. It needs to be translatable.

@stormrockwell
5 months ago

Fixed hard-coded text

#8 @stormrockwell
5 months ago

Could someone confirm that I am adding the translation functions correctly?

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


5 months ago

#10 @westonruter
5 months ago

@stormrockwell there should probably be a single string that gets used with substitutions. For example suggestedDimensions could look like: __( '%1$s by %2$s pixels' ) and then in JS you can replace those placeholders with the values for width and height respectively.

#11 @stormrockwell
5 months ago

  • Keywords has-patch added; needs-patch removed

Thank you very much @westonruter for the help. I reformatted 38932.3.patch following those tips.

#12 @westonruter
5 months ago

Patch looks good. The question of the hour then is how this gets milestoned. Does this go to 4.7.1 or 4.8?

#13 @ocean90
5 months ago

38932.3.patch is missing translator comments for the placeholders. /* translators: 1: ...


Does this go to 4.7.1 or 4.8?

I vote for 4.8

@stormrockwell
5 months ago

added translators comments

#14 @westonruter
5 months ago

  • Milestone changed from Future Release to 4.8

#15 @Presskopp
3 months ago

I made a new related ticket about adding units in media view:
#39667

Last edited 3 months ago by Presskopp (previous) (diff)

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


4 weeks ago

Note: See TracTickets for help on using tickets.