<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: Media controls accessibility improvement"
href="https://bugs.webkit.org/show_bug.cgi?id=160223#c27">Comment # 27</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: Media controls accessibility improvement"
href="https://bugs.webkit.org/show_bug.cgi?id=160223">bug 160223</a>
from <span class="vcard"><a class="email" href="mailto:eric.carlson@apple.com" title="Eric Carlson <eric.carlson@apple.com>"> <span class="fn">Eric Carlson</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=284866&action=diff" name="attach_284866" title="Patch">attachment 284866</a> <a href="attachment.cgi?id=284866&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=284866&action=review">https://bugs.webkit.org/attachment.cgi?id=284866&action=review</a>
I will be happy to r+ this with the minor nits noted. Thanks for keeping at this Nan!
<span class="quote">> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:892
> + var current = this.video.currentTime;
> + var step = this.timelineStepFromVideoDuration();</span >
Nit: both of these variables are unnecessary, each is only used once
<span class="quote">> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:900
> + var current = this.video.currentTime;
> + var step = this.timelineStepFromVideoDuration();</span >
Ditto.
<span class="quote">> 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;</span >
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();
<span class="quote">> LayoutTests/media/media-controls-accessibility.html:45
> + checkTimeLineValue();
> +
> + endTest();</span >
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.
<span class="quote">> LayoutTests/media/media-controls-accessibility.html:60
> + debug("timeline.value: " + timeline.value);
> + eventSender.keyDown("leftArrow");
> + debug("timeline.value: "+ timeline.value + "\n");</span >
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).</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>