Bug 67190 - A Media Element ie: <Audio> or <video>, without tabindex cannot be selected with keyboard (TAB Key).
Summary: A Media Element ie: <Audio> or <video>, without tabindex cannot be selected ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Unspecified
: P2 Normal
Assignee: Nobody
URL: http://yves.vg/testcases/webkit/video...
Keywords:
Depends on:
Blocks: 31786 68341
  Show dependency treegraph
 
Reported: 2011-08-30 02:53 PDT by Deepak Sherveghar
Modified: 2011-09-19 01:51 PDT (History)
8 users (show)

See Also:


Attachments
Proposed Patch. (2.81 KB, patch)
2011-08-30 03:00 PDT, Deepak Sherveghar
no flags Details | Formatted Diff | Diff
updated Patch. (2.06 KB, patch)
2011-08-30 03:10 PDT, Deepak Sherveghar
no flags Details | Formatted Diff | Diff
Patch (6.19 KB, patch)
2011-09-05 06:33 PDT, Deepak Sherveghar
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch (12.17 KB, patch)
2011-09-13 00:28 PDT, Deepak Sherveghar
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch (12.42 KB, patch)
2011-09-13 02:56 PDT, Deepak Sherveghar
tonikitoo: review+
tonikitoo: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (12.95 KB, patch)
2011-09-16 07:10 PDT, Deepak Sherveghar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Deepak Sherveghar 2011-08-30 02:53:02 PDT
Filing a new bug as mentioned by Alexey Proskuryakov in bug : https://bugs.webkit.org/show_bug.cgi?id=31786 

Steps:
1. Go to the URL : http://yves.vg/testcases/webkit/video.html
2. Hit the TAB key to access a video or audio element without tab-index.

Expected Result:
Media element should be focused.

Since the W3C Draft categorizes video and audio in "Interactive Content".
Link: http://www.w3.org/TR/html5/embedded-content-0.html#interactive-content
Comment 1 Deepak Sherveghar 2011-08-30 03:00:55 PDT
Created attachment 105605 [details]
Proposed Patch.
Comment 2 Deepak Sherveghar 2011-08-30 03:02:41 PDT
Override supportsfocus() and return true for HTMLMediaElement.
This is required to support focus for media elements without tab-index.
Media Elements like <audio> <video> should be focusable with keyboard (tab key) as per W3c Draft for Interactive Content video and audio.

Earlier even with spatial-navigation enabled (i.e. --enable-spatial-navigation=true), Media elements were not getting selected.
With this patch even that is working fine.
Comment 3 Deepak Sherveghar 2011-08-30 03:10:00 PDT
Created attachment 105607 [details]
updated Patch.
Comment 4 Eric Carlson 2011-08-30 07:45:55 PDT
Comment on attachment 105607 [details]
updated Patch.

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

> Source/WebCore/html/HTMLMediaElement.cpp:227
> +bool HTMLMediaElement::supportsFocus() const
> +{
> +    return true;
> +}

This is not correct, the spec says a media element is only interactive when it has the 'controls' attribute. From http://www.w3.org/TR/html5/embedded-content-0.html#interactive-content:

    Interactive content is content that is specifically intended for user interaction.

        => ... audio (if the controls attribute is present) ... video (if the controls attribute is present)
Comment 5 Alexey Proskuryakov 2011-08-30 09:12:29 PDT
Also, it's likely possible to make a regression test for this fix.
Comment 6 Ian 'Hixie' Hickson 2011-08-30 21:56:09 PDT
Whether something is "interactive content" or not has no bearing on whether it is focusable or not: the definition of focusable doesn't refer to whether something is "interactive content":

   http://www.whatwg.org/specs/web-apps/current-work/complete.html#focusable

Whether something should be focusable or not is essentially up to the browser vendor (you). I would recommend making it possible to focus video controls if they are present, of course. But that's not a spec conformance matter, it's a usability matter.
Comment 7 Antonio Gomes 2011-09-03 22:17:06 PDT
> Earlier even with spatial-navigation enabled (i.e. --enable-spatial-navigation=true), Media elements were not getting selected.
> With this patch even that is working fine.

if we could add a spatial navigation test, it'd be awesome.
Comment 8 Deepak Sherveghar 2011-09-05 06:33:36 PDT
Created attachment 106331 [details]
Patch
Comment 9 Antonio Gomes 2011-09-05 07:18:27 PDT
Comment on attachment 106331 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        override supportsFocus for HTMLMediaElement.

*Override

> Source/WebCore/ChangeLog:8
> +        Test: accessibility/media-element-focus-tab.html

What about the spatial navigation test? :)

> Source/WebCore/ChangeLog:10
> +        return true from supportsFocus() if control's attribute is present or a tabindex is specified.

*Return
Comment 10 WebKit Review Bot 2011-09-05 07:32:18 PDT
Comment on attachment 106331 [details]
Patch

Attachment 106331 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9595280

New failing tests:
fast/events/tabindex-focus-blur-all.html
Comment 11 Deepak Sherveghar 2011-09-05 11:21:06 PDT
(In reply to comment #9)
> (From update of attachment 106331 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106331&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        override supportsFocus for HTMLMediaElement.
> 
> *Override

Ok :) 
> 
> > Source/WebCore/ChangeLog:8
> > +        Test: accessibility/media-element-focus-tab.html
> 
> What about the spatial navigation test? :)

extremely sorry Antonio, have created it but forgot to submit with the patch :( .will submit a new patch along with your review comments incorporated.

> 
> > Source/WebCore/ChangeLog:10
> > +        return true from supportsFocus() if control's attribute is present or a tabindex is specified.
> 
> *Return

Ok :)
Comment 12 Deepak Sherveghar 2011-09-06 06:38:18 PDT
Regarding the failing test-case : LayoutTests/fast/events/tabindex-focus-blur-all.html
Failing cases are as follows:
1. In LayoutTests/fast/events/resources/tabindex-focus-blur-all-frame1.html, Audio element without tab-index.
<audio controls src="../../../media/content/test.wav" id="audio1"></audio><br>
2. In LayoutTests/fast/events/resources/tabindex-focus-blur-all-iframe2.html, Video element without tab-index.
<video controls id="video4">
  <source src="../../../media/content/test.mp4" type="video/mpeg"id="source4">
</video><br>

Test output for the above two element states that:
audio1 FAILED - was focused but focus wasn't expected
elemThatShouldFocus is null, focusedElem is <AUDIO> audio1

video4 FAILED - was focused but focus wasn't expected
elemThatShouldFocus is null, focusedElem is <VIDEO> video4

JS  LayoutTests/fast/events/resources/tabindex-focus-blur-all.js used in the test compares elem.tabIndex == elem.getAttribute("tabindex") to verify which element should be focused. In our case, media elements are focused as expected, but the above condition fails since no tab-index attribute is present on the media element and hence the FAILED messages. 

But media element should be focusable even though tab-index is not specified because control's attribute is present.

Not sure how to go about making this test case pass :( .
Any suggestions or pointers or approaches to resolve the above failing test case ???? 

On  a different note, I observed that without my patch, The expected results are little different.
ie: on my linux ubuntu machine with webkit GTK build, running the above test with DRT gave following output
===============================================================================
bpwv64@MIG-Workstation:~/projects/webkit_gtk/WebKit$ WebKitBuild/Debug/Programs/DumpRenderTree  LayoutTests/fast/events/tabindex-focus-blur-all.html
Gtk-Message: Failed to load module "canberra-gtk-module"
332 focus / 331 blur events dispatched, and should be 331 / 331 FAILED
Total of 0 focus test(s) failed. PASSED
#EOF
LEAK: 2889 WebCoreNode
LEAK: 2 CachedResource
bpwv64@MIG-Workstation:~/projects/webkit_gtk/WebKit$
===============================================================================
Comment 13 Hayato Ito 2011-09-11 21:12:18 PDT
(In reply to comment #12)
> 
> But media element should be focusable even though tab-index is not specified because control's attribute is present.
> 
> Not sure how to go about making this test case pass :( .
> Any suggestions or pointers or approaches to resolve the above failing test case ???? 

> But media element should be focusable even though tab-index is not specified because control's attribute is present.

If your argument is right, the only way to pass the test is to update LayoutTests/fast/events/tabindex-focus-blur-all.html and the expected txt .
It looks that the test assumes media element should not be focusable if tab-index is not present, which conflicts this patch, doesn't it?

As Ian said, that's not a spec conformance matter, it's a usability matter, I guess.
Comment 14 Deepak Sherveghar 2011-09-13 00:28:22 PDT
Created attachment 107149 [details]
Updated patch
Comment 15 WebKit Review Bot 2011-09-13 01:54:25 PDT
Comment on attachment 107149 [details]
Updated patch

Attachment 107149 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9657051

New failing tests:
fast/events/tabindex-focus-blur-all.html
Comment 16 Deepak Sherveghar 2011-09-13 02:56:58 PDT
Created attachment 107160 [details]
Updated patch
Comment 17 Antonio Gomes 2011-09-13 07:02:01 PDT
Patch looks good to me, but since it would be changing a general behavior of all webkit based browsers (and according to the spec it is app to the UA to implement that or not), we could have a discussion in webkit-dev mailing list first.

Well, at least that would be my way of approaching the problem.
Comment 18 Deepak Sherveghar 2011-09-14 04:14:04 PDT
(In reply to comment #17)
> Patch looks good to me, but since it would be changing a general behavior of all webkit based browsers (and according to the spec it is app to the UA to implement that or not), we could have a discussion in webkit-dev mailing list first.
> 
> Well, at least that would be my way of approaching the problem.

Agreed.
Done. https://lists.webkit.org/pipermail/webkit-dev/2011-September/017957.html
Comment 19 Ryosuke Niwa 2011-09-14 09:50:13 PDT
Comment on attachment 107160 [details]
Updated patch

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

> LayoutTests/fast/events/media-element-focus-tab.html:1
> +<p>This tests that a media element ie: <code>&lt;Audio&gt;</code> or <code>&lt;video&gt;</code>, without tabindex can be selected with keyboard (TAB Key)</p>

no DOCTYPE, html , & body?

> LayoutTests/fast/events/media-element-focus-tab.html:45
> +        document.getElementById("console").innerHTML +=  mediaFocusedMsg + " TEST: " + fieldId + ".\n";

Nit: two spaces after +=.

> LayoutTests/fast/events/media-element-focus-tab.html:54
> +test("WithTabIndexVideo", "SUCCESS: Tab-Key did tab to the Media Element (video1).", "FAIL: Tab-Key did not tab to the Media Element (video1).");
> +test("WithoutTabIndexVideo", "SUCCESS: Tab-Key did tab to the Media Element (video2).", "FAIL: Tab-Key did not tab to the Media Element (video2).");
> +test("WithoutControlsButWithTabIndexVideo", "SUCCESS: Tab-Key did tab to the Media Element (video3).", "FAIL: Tab-Key did not tab to the Media Element (video3).");
> +test("WithoutTabIndexAudio", "SUCCESS: Tab-Key did tab to the Media Element (audio1).", "FAIL: Tab-Key did not tab to the Media Element (audio1).");
> +test("WithoutControlaAndTabIndexAudio", "FAIL: Tab-Key did tab to the Media Element (audio2).", "SUCCESS: Tab-Key did not tab to the Media Element (audio2).");

Duplicating all these pass/fail messages seem inelegant to me.  You should be able to create one in test by string concatenations.

> LayoutTests/fast/spatial-navigation/snav-media-elements.html:1
> +<html>

No DOCTYPE?

> LayoutTests/fast/spatial-navigation/snav-media-elements.html:8
> +      ["Down", "v1"],

Maybe up should try up first?
Comment 20 Antonio Gomes 2011-09-15 07:31:25 PDT
Comment on attachment 107160 [details]
Updated patch

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

r=me. Please address Ryosuke's concerns before committing.

When you have a new patch, pre-fill "Reviewed by Antonio Gomes" field, and set cq? only. Then any committer can cq+ it.

> LayoutTests/fast/events/tabindex-focus-blur-all-expected.txt:1
> -331 focus / 331 blur events dispatched, and should be 331 / 331 PASSED
> +333 focus / 333 blur events dispatched, and should be 333 / 333 PASSED

this part looks suspicious.

> LayoutTests/fast/events/resources/tabindex-focus-blur-all.js:57
> -    var resultSummary = focusCount+" focus / "+blurCount+" blur events dispatched, and should be 331 / 331 ";
> +    var resultSummary = focusCount+" focus / "+blurCount+" blur events dispatched, and should be 333 / 333 ";

ditto.
Comment 21 Deepak Sherveghar 2011-09-16 07:10:37 PDT
Created attachment 107644 [details]
Updated Patch
Comment 22 WebKit Review Bot 2011-09-17 03:56:23 PDT
Comment on attachment 107644 [details]
Updated Patch

Clearing flags on attachment: 107644

Committed r95381: <http://trac.webkit.org/changeset/95381>
Comment 23 WebKit Review Bot 2011-09-17 03:56:31 PDT
All reviewed patches have been landed.  Closing bug.