Improve showing metadata in media view

[Presskopp]

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"

[Presskopp]

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

related: #38932

[kiranpotphode]

Patch for adding units to numbers.

[Presskopp]

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

[milindmore22]

Patch will add units to media modal box

[stormrockwell]

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.

[adamsilverstein]

@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)

[milindmore22]

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.

[stormrockwell]

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).

[milindmore22]

Should we ask this in #accessibility slack channel? 🤔

[milindmore22]

added screen reader stuff and introduced new function duration_format

[milindmore22]

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

[milindmore22]

Fixed notices and added regular expression for validation

[Presskopp]

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]

Updated with test case improved coding with phpcs

[milindmore22]

Fixed spelling mistake in comment

[adamsilverstein]

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

[afercia]

@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

[adamsilverstein]

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

[adamsilverstein]

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.

[adamsilverstein]

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)

[afercia]

@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]

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

[milindmore22]

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'] );

[afercia]

@adamsilverstein discussed this ticket during today's a11y bug-scrub and wondering: anything left to do here? Thanks!

[adamsilverstein]

@afercia seems like this is in a good place to me. I would only refresh the patch and verify still working then commit!

[afercia]

39667.4.diff​ is a refreshed patch with a few changes:

Ideally, aria-label should be used on ​labelable elements. When used on other elements (previous patches used it on a span element), assistive technologies expect that element to have a native "role" (or an ARIA role). Since no ARIA role was set in the markup (and there's no applicable role), assistive technologies may announce something unexpected; VoiceOver, for example, fallbacks to a generic "group" role and announced the span element as "group". To avoid this, I've opted to use some screen-reader-text instead of an aria-label.

I've applied the same enhancements to the attachment details in the "Add Media" modal.

Note: the patch removes some unrelated trailing spaces (my editor does that automatically).

Looks good to me, @adamsilverstein please do feel free to review and commit when you have a chance :)

[adamsilverstein]

Running tests one more time. 39667.6.diff​ fixes the array setup for earlier PHP versions

[adamsilverstein]

In 43633:

Media: Improve display and accessibility of meta data in detail view.

• Add a human_readable_duration function including tests.
• Add 'pixels' after image width/height.
• Add screen reader text for durations.

Props Presskopp, kiranpotphode, milindmore22, stormrockwell, afercia.
Fixes #39667.

[birgire]

+1 for these changes.

Just some late afterthoughts:

Regarding the human_readable_duration(), I was wondering about the out of bound values, negative playstrings and if 0-parts should be generated.

It seems that the maximum duration supported by that function is 39:59:59. It comes from the reg-ex. I doubt it will be ever passed in reality, but maybe we should document it in the functions description?

So for documentation purpose, it might be helpful to have the out of bound behavior listed in the data provider e.g. as:

array( '39:59:59', '39 hours, 59 minutes, 59 seconds' ), // Maximum support for HH:ii:ss.
array( '59:60', false ), // Out of bound seconds for ii:ss.
array( '60:59', false ),  // Out of bound minutes for ii:ss.
array( '3:59:60', false ), // Out of bound seconds for HH:ii:ss.
array( '3:60:59', false ), // Out of bound minutes for HH:ii:ss.
array( '40:00:00', false ), // Out of bound hours for HH:ii:ss.

I looked at getid3_lib::PlaytimeString() but didn't see any explicit hour limitations there:

https://core.trac.wordpress.org/browser/tags/4.9.8/src//wp-includes/ID3/getid3.lib.php#L461

It also seems to be able to generate negative playstrings, should that be considered here?

I first thought the PHP function data_parse() could be useful here, but it has some differences, like it understands 12:34 as HH:ii, but not as ii:ss. So I think it might just complicate things more using that function :-)

Another thing that comes to mind, is that if 0-parts would be needed in the generated string or if it should be an option?

Example 00:00:01 is generated as 0 hours, 0 minutes, 1 second but would it make sense to generate only 1 second? But then it's not obvious what string 00:00:00 should generate :-)

If I remember correctly human_time_diff() doesn't display 0-parts? Or maybe we shouldn't do such a comparison here, as it's not exactly the same kind of data.

[adamsilverstein]

@birgire Thanks for the additional feedback and review.

It seems that the maximum duration supported by that function is 39:59:59. It comes from the reg-ex. I doubt it will be ever passed in reality, but maybe we should document it in the functions description?

Hmmm, didn't realize that. It would be good to support a larger number of hours, can we fix that? (feel free to reopen with improvements)

It also seems to be able to generate negative playstrings, should that be considered here?

Worth testing, seems like an edge case.

Example 00:00:01 is generated as 0 hours, 0 minutes, 1 second but would it make sense to generate only 1 second? But then it's not obvious what string 00:00:00 should generate :-)

If I remember correctly human_time_diff() doesn't display 0-parts? Or maybe we shouldn't do such a comparison here, as it's not exactly the same kind of data.

The purpose/use case for this function is to provide clear screen reader/assistive text. We need to describe the zeros if they are passed to the function - however in practice the media modal *already* strips off prefix zeros before passing the duration string to this function, so there won't be any zeros being passed in the media modal use.

[birgire]

thanks @adamsilverstein

I noticed that for '0:5' and '0:05', the function's output is '5 seconds' instead of '0 minutes, 5 seconds'.

39667.7.diff​ is a suggestion that:

• Removes the upper limit of 39:59:59 for the input duration.
• Splits the regex into two separate cases: HH:ii:ss and ii:ss, to simplify it. This previous validation part: (([0-3]?[0-9])|([2][0-3])) was not obvious to me.
• Removes negative duration sign, like '-32:49', as possibly prepended by getid3_lib::PlaytimeString().
• Adds trim to the input duration.
• Uses 'duration' instead of 'filelength' in inline docs and variables. A suggestion to relate it more with the function's name human_readable_duration().
• The input duration '0:5' and '0:05' now outputs as '0 minutes, 5 seconds' instead of '5 seconds'.
• Adds more test cases (e.g. for out of bound).
• Adds inline docs to the dataprovider and the test method.
• Adjusts the name of the test methods.
• Changes from @return boolean|string to @return string|false as it never returns true. Makes this change sense?
• Changes @since 5.0 to @since 5.0.0.

Regarding a new function's name, are there any guidelines when the wp_ prefix should be used?

[adamsilverstein]

@birgire Thank you for the enhancements & fixes! I'll take a look soon and follow up soon. Regarding the naming, I haven't discerned a pattern, I think the original naming here came from the similarity to human_time_diff

[peterwilsoncc]

Moving to the 5.1 milestone due to the WordPress 5.0 focus on the new
editor (Gutenberg).

[pento]

@adamsilverstein: Are you able to review/commit 39667.7.diff​ before 5.1 beta 1?

[adamsilverstein]

@pento yes, i'll take a look tonight or tomorrow.

[adamsilverstein]

looks great, thanks for adding the additional test coverage @birgire

39667.8.diff​ is slightly updated with @since 5.1.0.

I think the naming is fine, we can stick with that for now unless there are objections.

[adamsilverstein]

Going to commit this as soon as all tests pass in Travis. @pento do we need a dev note for this new function?

[adamsilverstein]

In 44481:

Media: improve the human_readable_duration function and tests.

Improve the human_readable_duration added in #39667:

• Remove upper limit.
• More resilient handling: remove negative prefix, trim.
• Correct @since to 5.1.0.
• Adds more test cases and improve inline docs.

Props birgire.
Fixes #39667.

[pento]

@adamsilverstein: A short dev note would be wonderful, thank you. :)

[geminorum]

great, but lacks the translation of the final comma. :D

[desrosj]

Dev note has been posted: ​https://make.wordpress.org/core/2019/01/15/new-function-human_readable_duration/