Bug 160223 – AX: Media controls accessibility improvement

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Nan Wang]

Couple issues right now:
1. VoiceOver users adjusting volume slider with VO+arrow keys does not have effect.
2. The parent timer of elapsed/remaining time should have the time value in the label
3. VO-left and right should be larger jumps on timeline slider
4. VO-space on mute button always says unmuting.

[Radar WebKit Bug Importer]

<rdar://problem/27558003>

[Nan Wang]

Created attachment 284654 [details]
patch

[Nan Wang]

Comment on attachment 284654 [details]
patch

going to fixed the failed tests

[Nan Wang]

Created attachment 284662 [details]
patch

Fix tests

[Build Bot]

Comment on attachment 284662 [details]
patch

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

New failing tests:
media/media-document-audio-repaint.html
media/controls-drag-timebar.html

[Build Bot]

Created attachment 284663 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 284662 [details]
patch

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

New failing tests:
media/media-document-audio-repaint.html
media/controls-drag-timebar.html

[Build Bot]

Created attachment 284664 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 284662 [details]
patch

Attachment 284662 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1759323

New failing tests:
media/media-document-audio-repaint.html
media/controls-drag-timebar.html

[Build Bot]

Created attachment 284665 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Nan Wang]

Created attachment 284667 [details]
patch

fix tests

[Eric Carlson]

Comment on attachment 284667 [details]
patch

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

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1182
> +        if (this.video.muted) {
> +            this.video.muted = false;
> +            this.controls.muteButton.setAttribute('aria-checked', 'false');
> +        }
> +        this.video.volume = this.controls.volume.value;
> +        this.controls.volume.setAttribute('aria-valuetext', parseInt(this.controls.volume.value * 100) + '%');

It is unfortunate to have yet another function that checks/changes this.video.muted, sets this.video.volume,  and sets the aria-valuetext. Can you make a helper function that does these common steps and call it from your new event handler and the existing event handler that have duplicate code?

[Nan Wang]

Created attachment 284671 [details]
patch

update from review

[Eric Carlson]

Comment on attachment 284671 [details]
patch

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

r- for now because of the hard coded timeline step value and the long/flaky test.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:424
> +        timeline.step = 1;

While 1 will be a better value than .01 for some movies, it will be much worse for very short files, and it still won't be great for long files. Can you set it to a percentage of the duration in updateDuration instead? You may want to vary the percentage based on duration, e.g. 1% of a 90 second file is almost a second but 1% of a 90 minute is almost a minute, but a I'll bet you can come up with reasonable values with some experimentation.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1727
> +        this.controls.currentTime.setAttribute('aria-label', this.UIString('Elapsed') + ' ' + this.formatTime(currentTime));

Nit: A template literal could work well here:

this.controls.currentTime.setAttribute('aria-label', `${this.UIString('Elapsed')} ${this.formatTime(currentTime)}`);

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:2104
> +        this.controls.volume.setAttribute('aria-valuetext', parseInt(this.controls.volume.value * 100) + '%');

Nit: A template literal could work well here:

this.controls.currentTime.setAttribute('aria-label', `${this.UIString('Remaining')} ${this.formatTime(timeRemaining)}`);

> LayoutTests/media/audio-controls-timeline-in-media-document.html:21
> +        setTimeout("testTimeLineValue()", 1000);

This test now takes a full second, which is a long time for a simple test like this. Additionally, the timeout is likely to make the test flaky. This won't be necessary if you don't always use a 1 second step value.

[Nan Wang]

Comment on attachment 284671 [details]
patch

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

>> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:424
>> +        timeline.step = 1;
> 
> While 1 will be a better value than .01 for some movies, it will be much worse for very short files, and it still won't be great for long files. Can you set it to a percentage of the duration in updateDuration instead? You may want to vary the percentage based on duration, e.g. 1% of a 90 second file is almost a second but 1% of a 90 minute is almost a minute, but a I'll bet you can come up with reasonable values with some experimentation.

We have come up with some cases that, let me know your thought:
1. If video is less then 10sec we do 0.5 second step
2. If video is between 10sec to 1min we do 1 second step
3. If video is between 1min to 10min we do 10 second step
4. If video is between 10min to 60min we do 30 second step
5. If video is more than 60min we do 60 second step.

>> LayoutTests/media/audio-controls-timeline-in-media-document.html:21
>> +        setTimeout("testTimeLineValue()", 1000);
> 
> This test now takes a full second, which is a long time for a simple test like this. Additionally, the timeout is likely to make the test flaky. This won't be necessary if you don't always use a 1 second step value.

I think the test video is 6 seconds long so based on the above cases, this will result into a .5 second step. Do you think it's acceptable for a 0.5 second test? Problem here is that from accessibility perspective, 0.01 step is not reasonable for a keyboard user as well. If the 0.5 second timeout is still not good, can you suggest us with a better way of modifying the test?

[Eric Carlson]

Comment on attachment 284671 [details]
patch

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

>>> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:424
>>> +        timeline.step = 1;
>> 
>> While 1 will be a better value than .01 for some movies, it will be much worse for very short files, and it still won't be great for long files. Can you set it to a percentage of the duration in updateDuration instead? You may want to vary the percentage based on duration, e.g. 1% of a 90 second file is almost a second but 1% of a 90 minute is almost a minute, but a I'll bet you can come up with reasonable values with some experimentation.
> 
> We have come up with some cases that, let me know your thought:
> 1. If video is less then 10sec we do 0.5 second step
> 2. If video is between 10sec to 1min we do 1 second step
> 3. If video is between 1min to 10min we do 10 second step
> 4. If video is between 10min to 60min we do 30 second step
> 5. If video is more than 60min we do 60 second step.

Now that I have played around with this, I don't think .step is the right way to fix this, is it possible to fix this problem with something else? While .step makes the slider more usable from the keyboard, it also makes it almost unusable as a media timeline slider because it is not possible to seek to specific times: e.g. in an hour long video it is only possible to seek to every minute. 

Play around with this fiddle to see what I mean: https://jsfiddle.net/jvL5nnnd

[chris fleizach]

(In reply to comment #16)
> Comment on attachment 284671 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284671&action=review
> 
> >>> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:424
> >>> +        timeline.step = 1;
> >> 
> >> While 1 will be a better value than .01 for some movies, it will be much worse for very short files, and it still won't be great for long files. Can you set it to a percentage of the duration in updateDuration instead? You may want to vary the percentage based on duration, e.g. 1% of a 90 second file is almost a second but 1% of a 90 minute is almost a minute, but a I'll bet you can come up with reasonable values with some experimentation.
> > 
> > We have come up with some cases that, let me know your thought:
> > 1. If video is less then 10sec we do 0.5 second step
> > 2. If video is between 10sec to 1min we do 1 second step
> > 3. If video is between 1min to 10min we do 10 second step
> > 4. If video is between 10min to 60min we do 30 second step
> > 5. If video is more than 60min we do 60 second step.
> 
> Now that I have played around with this, I don't think .step is the right
> way to fix this, is it possible to fix this problem with something else?
> While .step makes the slider more usable from the keyboard, it also makes it
> almost unusable as a media timeline slider because it is not possible to
> seek to specific times: e.g. in an hour long video it is only possible to
> seek to every minute. 
> 
> Play around with this fiddle to see what I mean:
> https://jsfiddle.net/jvL5nnnd

This was my concern too. Right now we're either making it impossible for KB users to use it or making it useless as a slider

Can we add a keyboard event handler for left/right arrow keys that will adjust the value appropriately while leaving the step as it currently is?

[Nan Wang]

Created attachment 284829 [details]
patch

Reverted the step change and added a keydown handler for that.
Not sure how to do that in test, I tried to timeline.focus() and then send a keydown event but that didn't work.

[Build Bot]

Comment on attachment 284829 [details]
patch

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

New failing tests:
media/media-controls-timeline-updates-after-playing.html

[Build Bot]

Created attachment 284837 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 284829 [details]
patch

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

New failing tests:
media/media-controls-timeline-updates-after-playing.html

[Build Bot]

Created attachment 284838 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 284829 [details]
patch

Attachment 284829 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1770022

New failing tests:
media/media-controls-timeline-updates-after-playing.html

[Build Bot]

Created attachment 284841 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Nan Wang]

Created attachment 284845 [details]
Patch

fixed test

[Nan Wang]

Created attachment 284866 [details]
Patch

Added test for the left/right arrow event.

[Eric Carlson]

Comment on attachment 284866 [details]
Patch

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

I will be happy to r+ this  with the minor nits noted. Thanks for keeping at this Nan!

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:892
> +        var current = this.video.currentTime;
> +        var step = this.timelineStepFromVideoDuration();

Nit: both of these variables are unnecessary, each is only used once

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:900
> +        var current = this.video.currentTime;
> +        var step = this.timelineStepFromVideoDuration();

Ditto.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1178
> +        var value = this.controls.timeline.value;
> +        switch(event.keyCode) {
> +        case this.KeyCodes.left:
> +                value = this.decrementTimelineValue();
> +            break;
> +        case this.KeyCodes.right:
> +                value = this.incrementTimelineValue();
> +            break;
> +        case this.KeyCodes.up:
> +        case this.KeyCodes.down:
> +            break;
> +        }
> +        this.controls.timeline.value = value;

As structured, this sets "this.controls.timeline.value = this.controls.timeline.value" for every keydown except left and right arrows. It will be more efficient with a simple if..else, something like:

    if (event.keyCode == this.KeyCodes.left) {
        this.controls.timeline.value = this.decrementTimelineValue();
    else if (event.keyCode == this.KeyCodes.right) {
        this.controls.timeline.value = this.incrementTimelineValue();

> LayoutTests/media/media-controls-accessibility.html:45
> +            checkTimeLineValue();
> +            
> +            endTest();

To make this test slightly more complete, you could a add 'seeked' event handler and call endTest from that to ensure that the arrow key actually results in a media timeline change.

> LayoutTests/media/media-controls-accessibility.html:60
> +        debug("timeline.value: " + timeline.value);
> +        eventSender.keyDown("leftArrow");
> +        debug("timeline.value: "+ timeline.value + "\n");

Because timeline.value is a floating point number, this is likely to print slightly different values on different versions of the OS. I recommend printing it with one digit of precision: timeline.value.toFixed(1).

[Nan Wang]

Created attachment 284869 [details]
patch

Thanks Eric!

updated from review.

[Eric Carlson]

Comment on attachment 284869 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 284869 [details]
patch

Clearing flags on attachment: 284869

Committed r203913: <http://trac.webkit.org/changeset/203913>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.