Bug 160619 - AX: Media controls timeline should have percentage value description
Summary: AX: Media controls timeline should have percentage value description
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-08-05 16:36 PDT by Nan Wang
Modified: 2016-08-10 16:31 PDT (History)
8 users (show)

See Also:


Attachments
patch (10.10 KB, patch)
2016-08-05 16:43 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (11.76 KB, patch)
2016-08-06 18:24 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (13.92 KB, patch)
2016-08-09 20:02 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (13.82 KB, patch)
2016-08-10 11:22 PDT, Nan Wang
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (905.14 KB, application/zip)
2016-08-10 12:13 PDT, Build Bot
no flags Details
Patch (13.83 KB, patch)
2016-08-10 12:31 PDT, Nan Wang
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (943.31 KB, application/zip)
2016-08-10 13:21 PDT, Build Bot
no flags Details
patch (14.15 KB, patch)
2016-08-10 13:40 PDT, Nan Wang
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nan Wang 2016-08-05 16:36:08 PDT
VoiceOver is speaking the seconds value when landing on the timeline control.

<rdar://problem/27645753>
Comment 1 Nan Wang 2016-08-05 16:43:32 PDT
Created attachment 285461 [details]
patch
Comment 2 chris fleizach 2016-08-05 17:00:44 PDT
Comment on attachment 285461 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285461&action=review

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1581
> +            return `${intHours} ${this.UIString('Hours')}, ${intMinutes} ${this.UIString('Minutes')}, ${intSeconds} ${this.UIString('Seconds')}`;

Does this take care of singular vs. Plural

> LayoutTests/platform/efl/accessibility/media-element-expected.txt:24
> +            title: AXTitle: Elapsed 0 Seconds

We should add cases for minutes and hours as well
As well as singular vs. Plural
Comment 3 Nan Wang 2016-08-05 17:03:01 PDT
Comment on attachment 285461 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285461&action=review

>> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1581
>> +            return `${intHours} ${this.UIString('Hours')}, ${intMinutes} ${this.UIString('Minutes')}, ${intSeconds} ${this.UIString('Seconds')}`;
> 
> Does this take care of singular vs. Plural

Nope. Will add that.

>> LayoutTests/platform/efl/accessibility/media-element-expected.txt:24
>> +            title: AXTitle: Elapsed 0 Seconds
> 
> We should add cases for minutes and hours as well
> As well as singular vs. Plural

Ok, will do.
Comment 4 Nan Wang 2016-08-05 17:18:28 PDT
Comment on attachment 285461 [details]
patch

Going to fix test and review comments.
Comment 5 Nan Wang 2016-08-06 18:24:21 PDT
Created attachment 285511 [details]
patch

Added code to cover singular and plural time units. Also added test cases. 
Not sure how to add test cases to cover minutes and hours without including a large video.
Comment 6 Eric Carlson 2016-08-08 08:15:57 PDT
(In reply to comment #5)
> Created attachment 285511 [details]
> patch
> 
> Added code to cover singular and plural time units. Also added test cases. 
> Not sure how to add test cases to cover minutes and hours without including
> a large video.

You could generate a VOD HLS stream with a little bit of PHP.  LayoutTests/http/tests/media/resources/hls/test-live.php generates a live stream, but you should be able to do something similar that generates a fixed duration stream based on a parameter passed from script.
Comment 7 Nan Wang 2016-08-08 08:51:54 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Created attachment 285511 [details]
> > patch
> > 
> > Added code to cover singular and plural time units. Also added test cases. 
> > Not sure how to add test cases to cover minutes and hours without including
> > a large video.
> 
> You could generate a VOD HLS stream with a little bit of PHP. 
> LayoutTests/http/tests/media/resources/hls/test-live.php generates a live
> stream, but you should be able to do something similar that generates a
> fixed duration stream based on a parameter passed from script.

Thanks Eric! Will look into that.
Comment 8 Nan Wang 2016-08-09 20:02:10 PDT
Created attachment 285708 [details]
patch

updated tests
Comment 9 Eric Carlson 2016-08-09 22:00:32 PDT
Comment on attachment 285708 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285708&action=review

> LayoutTests/ChangeLog:10
> +        * http/tests/media/resources/hls/test-video-duration-accessibility.php: Added.

This file isn't accessibility-specific, how about something like "generate-vod.php"?

> LayoutTests/http/tests/media/hls/video-duration-accessibility.html:36
> +                // We are not getting the exact same elapsed duration for hour cases, 
> +                // so let's special case this.

What does this mean, is it something we need to fix?

> LayoutTests/http/tests/media/hls/video-duration-accessibility.html:42
> +                if (elapsedTimer.description.indexOf("Hour") !== -1) {
> +                    testExpected("elapsedTimer.description.indexOf('1 Hour') !== -1 || elapsedTimer.description.indexOf('2 Hours') !== -1", true);
> +                } else {
> +                    consoleWrite("elapsedTimer.description: " + elapsedTimer.description);
> +                }
> +                

Nit: WebKit style is to not use braces for single line if statements.

> LayoutTests/http/tests/media/hls/video-duration-accessibility.html:47
> +                if (seekCount == seekTimes.length) {
> +                    endTest();
> +                } else {
> +                    video.fastSeek(seekTimes[seekCount]);
> +                }

Ditto.

> LayoutTests/http/tests/media/resources/hls/test-video-duration-accessibility.php:10
> +// header("Content-Length: " . filesize(__FILE__));

Nit: this isn't necessary.

> LayoutTests/http/tests/media/resources/hls/test-video-duration-accessibility.php:13
> +$chunkDuration = 6.0272;
> +$chunkCount = 1300;

I would prefer that the duration of the file come from an optional parameter so we can use this for other tests in the future. Something like (untested):

    $chunkCount = 1300;
    if (array_key_exists("duration", $_GET))
        $chunkCount = $_GET["duration"] / $chunkDuration;
Comment 10 Nan Wang 2016-08-09 22:56:06 PDT
Comment on attachment 285708 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285708&action=review

Thanks for the review, will address those issues.

>> LayoutTests/http/tests/media/hls/video-duration-accessibility.html:36
>> +                // so let's special case this.
> 
> What does this mean, is it something we need to fix?

So when I have the fixed large number passed into fastSeek(), like 3600, I'm not getting the same elapsed time each time when I run the test. I guess it's something to do with the #EXTINF is not exactly the same as the duration of the .ts file?
Comment 11 Nan Wang 2016-08-10 11:22:50 PDT
Created attachment 285734 [details]
patch

Updated from review
Comment 12 Build Bot 2016-08-10 12:13:50 PDT
Comment on attachment 285734 [details]
patch

Attachment 285734 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1847255

New failing tests:
http/tests/media/hls/video-duration-accessibility.html
Comment 13 Build Bot 2016-08-10 12:13:55 PDT
Created attachment 285744 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 14 Eric Carlson 2016-08-10 12:19:48 PDT
Comment on attachment 285734 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285734&action=review

> LayoutTests/http/tests/media/resources/hls/generate-vod.php:14
> +$chunkCount = 0;
> +if (array_key_exists("duration", $_GET)) 
> +    $chunkCount = $_GET["duration"] / $chunkDuration;

This makes the "duration" param optional, so $chunkCount should have a default value that will result in a playable stream.
Comment 15 Nan Wang 2016-08-10 12:31:45 PDT
Created attachment 285745 [details]
Patch

Added default value for chunkCount.
Comment 16 Nan Wang 2016-08-10 13:20:48 PDT
(In reply to comment #14)
> Comment on attachment 285734 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285734&action=review
> 
> > LayoutTests/http/tests/media/resources/hls/generate-vod.php:14
> > +$chunkCount = 0;
> > +if (array_key_exists("duration", $_GET)) 
> > +    $chunkCount = $_GET["duration"] / $chunkDuration;
> 
> This makes the "duration" param optional, so $chunkCount should have a
> default value that will result in a playable stream.

Seems this test is not passing on mac-wk2. But it passed on my local machine. Do you have any idea what could be the cause? The previous patch without the duration parameter seemed working fine.
Comment 17 Build Bot 2016-08-10 13:21:48 PDT
Comment on attachment 285745 [details]
Patch

Attachment 285745 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1847524

New failing tests:
http/tests/media/hls/video-duration-accessibility.html
Comment 18 Build Bot 2016-08-10 13:21:51 PDT
Created attachment 285750 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 19 Nan Wang 2016-08-10 13:23:38 PDT
(In reply to comment #16)
> (In reply to comment #14)
> > Comment on attachment 285734 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=285734&action=review
> > 
> > > LayoutTests/http/tests/media/resources/hls/generate-vod.php:14
> > > +$chunkCount = 0;
> > > +if (array_key_exists("duration", $_GET)) 
> > > +    $chunkCount = $_GET["duration"] / $chunkDuration;
> > 
> > This makes the "duration" param optional, so $chunkCount should have a
> > default value that will result in a playable stream.
> 
> Seems this test is not passing on mac-wk2. But it passed on my local
> machine. Do you have any idea what could be the cause? The previous patch
> without the duration parameter seemed working fine.

There seemed to be a stalled event when it tried to seek for 1 hour duration.
Comment 20 Nan Wang 2016-08-10 13:25:22 PDT
(In reply to comment #18)
> Created attachment 285750 [details]
> Archive of layout-test-results from ews104 for mac-yosemite-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-wk2-ews.
> Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

For the new failure we are not getting the exact same minute duration as expected.
Comment 21 Nan Wang 2016-08-10 13:40:27 PDT
Created attachment 285755 [details]
patch

Updated test
Comment 22 Eric Carlson 2016-08-10 16:22:12 PDT
Comment on attachment 285755 [details]
patch

Thanks for keeping this Nan!
Comment 23 Nan Wang 2016-08-10 16:31:54 PDT
Committed r204361: <http://trac.webkit.org/changeset/204361>