WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 4 months ago

#39667 assigned enhancement

Improve showing metadata in media view

Reported by: Presskopp Owned by: adamsilverstein
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Media Keywords: has-screenshots has-patch dev-feedback has-unit-tests
Focuses: accessibility Cc:

Description

I'd like to see units after numbers here, see screenshot.

This is the view you get after clicking a video in media gallery (Attachment Details) and then click on "Edit more details"

Attachments (10)

media.jpg (29.6 KB) - added by Presskopp 7 months ago.
39667.diff (1005 bytes) - added by kiranpotphode 7 months ago.
Patch for adding units to numbers.
media-patched.jpg (30.7 KB) - added by Presskopp 7 months ago.
39667-1.diff (1.1 KB) - added by milindmore22 7 months ago.
Patch will add units to media modal box
39667-2.diff (4.0 KB) - added by milindmore22 4 months ago.
added screen reader stuff and introduced new function duration_format
39667-3.diff (4.6 KB) - added by milindmore22 4 months ago.
Fixed notices and added regular expression for validation
39667-4.diff (5.6 KB) - added by milindmore22 4 months ago.
Updated with test case improved coding with phpcs
39667-5.diff (5.6 KB) - added by milindmore22 4 months ago.
Fixed spelling mistake in comment
39667.2.diff (5.7 KB) - added by adamsilverstein 4 months ago.
39667.3.diff (5.7 KB) - added by milindmore22 4 months ago.
Corrected function name in latest cleaned up patch, file media.php

Download all attachments as: .zip

Change History (33)

@Presskopp
7 months ago

#1 @Presskopp
7 months ago

sorry about the typo in Length *X* - that was me

related: #38932

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

#2 @adamsilverstein
7 months ago

  • Keywords good-first-bug added

@kiranpotphode
7 months ago

Patch for adding units to numbers.

#3 @kiranpotphode
7 months ago

  • Keywords has-patch added

#4 @Presskopp
7 months ago

  • Keywords has-screenshots added

thanks @kiranpotphode , screenshot with patch active. What about screenreader stuff?

#5 @adamsilverstein
7 months ago

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

@milindmore22
7 months ago

Patch will add units to media modal box

#6 @stormrockwell
7 months ago

For the dimensions, I think it makes sense to follow the format 200 by 200 pixels like ticket #38932 since we are planning to move forward with that format in 4.8 for suggested image dimensions.

Does min reflect the time format 00:00:00 correctly? For example, if it was 1:02:00 it would have to read 62 min to be correct or change it to 1:02:00 hours. I think adding a unit for the time layout might be a little confusing.

Last edited 7 months ago by stormrockwell (previous) (diff)

#7 follow-up: @adamsilverstein
5 months ago

@milindmore22 Thanks for the patch!

These strings need to be translatable. Can you take a pass at applying the same approach as #38932 here? (the last patch)

#8 @adamsilverstein
5 months ago

  • Keywords needs-patch added; has-patch removed

#9 in reply to: ↑ 7 @milindmore22
5 months ago

Hello @adamsilverstein,

Thanks for reviewing my patch

I am confused about file duration please check case explained by @stormrockwell above what will be appropriate screen reader content.

Will it be 1:02:00 hours OR 62:00 mins OR 01 Hour 02 minutes 00 seconds

I am thinking of something like humanreadableFileLength with function duration_format()

<span aria-label="01 Hour 02 minutes 00 seconds">1:02:00</span>

please let me know your thoughts and I will work on patch to fix it.

#10 @stormrockwell
5 months ago

In my opinion, I don't think time should have a unit because there isn't a unit I can think of for the time format "hh:mm:ss.sss".

I like the idea of the aria-label above (I would just recommend removing the leading zeros).

#11 @milindmore22
4 months ago

Should we ask this in #accessibility slack channel? 🤔

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


4 months ago

@milindmore22
4 months ago

added screen reader stuff and introduced new function duration_format

#13 @milindmore22
4 months ago

  • Focuses accessibility added
  • Keywords has-patch dev-feedback reporter-feedback added; needs-patch removed

added a new patch with updates from comment #comment:9 and #comment:10 please let me know your thoughts.

@milindmore22
4 months ago

Fixed notices and added regular expression for validation

#14 @Presskopp
4 months ago

  • Keywords good-first-bug reporter-feedback removed

I'd like to have patched this myself, but I saw it coming it's not that trivial. Can't say what is needed, nice to have, or overkill.

@milindmore22
4 months ago

Updated with test case improved coding with phpcs

@milindmore22
4 months ago

Fixed spelling mistake in comment

#15 @milindmore22
4 months ago

  • Keywords has-unit-tests added

#16 @adamsilverstein
4 months ago

cc: @afercia can you provide some feedback regarding the strings passed to screen readers? (https://core.trac.wordpress.org/ticket/39667#comment:9) - thanks!

#17 @afercia
4 months ago

@adamsilverstein I'm not sure that would be feasible or approachable, considering all this should also be translatable...
By the way, the point is screen readers are often unable to get what specific format is, so they will announce something like 1:02:00 just as numbers and the colons doing a little pause:
one ... zero two ... zero zero.
thus making difficult to understand what that means. Providing an alternative text, in this case by the means of an aria-label, would be great and would allow to use a string in a more natural, spoken-like, language. I mean, as you would normally say that in your every-day English :) for example"
one hour, two minutes, zero seconds.
I have no idea how to do that programmatically though.

The size case is a bit easier. I've done something similar in another project recently, just avoiding to use abbreviations. Also, depending on what character is used for the "x", the size may be read out as:
384 /ˈɛks/ 216 pee /ˈɛks/ or 384 times 216 pee /ˈɛks/
Simply using a natural language would solve the issue:
384 by 216 pixels

#18 @adamsilverstein
4 months ago

@afercia thank you for the feedback.

The part about providing natural language and avoiding abbreviations for the screen reader makes perfect sense. Since we want to keep the displayed text concise, this likely means separate strings for display vs. speaking (which makes sense to me in this case).

In terms of how to announce something like 1:05:12 I think we should leave this up to the screen readers? Trying to accomplish this manually seems tricky, especially with local variations in plurals or even the use of am/pm.

#19 @adamsilverstein
4 months ago

Reviewing the latest patch I see that @milindmore22 already added code to provide a human readable duration value for the aria label. I am going to clean up that patch a little and provide some examples here.

#20 @adamsilverstein
4 months ago

39667.2.diff is a slight cleanup of 39667-5.diff, changes include:

  • clean up documentation
  • move tests to functions.php, add one test to match discussed example
  • fix conditional in first logic check (|| not &&)
  • rename helper function to human_readable_duration

This could use some additional testing (especially with screen readers) and also screenshots showing the results. Is the aria-label announced when it changes, or do we need to call wp.a11y.speak explicitly?

@afercia you can see the way the human_readable_duration function works by looking at the unit test in the patch. I added the test example you gave: array( '1:02:00', '1 hour, 2 minutes, 0 seconds' ) (input, output)

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


4 months ago

#22 @afercia
4 months ago

  • Milestone changed from Awaiting Review to Future Release

@adamsilverstein I like the approach! Wondering if it could be abstracted and be a human_readable_time thing, to be re-used in other places too. Thinking at the General Settings page, for example. And yes, also a human_readable_date would be very useful :)

@milindmore22
4 months ago

Corrected function name in latest cleaned up patch, file media.php

#23 @milindmore22
4 months ago

Thanks for cleanup

Fixed and updated latest cleaned patch seems missing correct function name in media.php

instead of

$response['fileLengthHumanReadable'] = duration_format( $meta['length_formatted'] );

added

$response['fileLengthHumanReadable'] = human_readable_duration( $meta['length_formatted'] );

Note: See TracTickets for help on using tickets.