[Webkit-unassigned] [Bug 160223] AX: Media controls accessibility improvement

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=160223

--- Comment #27 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 284866
  --> https://bugs.webkit.org/attachment.cgi?id=284866
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).

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160729/1d21dd6c/attachment.html>