Bug 67190

Created attachment 105605 [details] Proposed Patch.

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.

Created attachment 105607 [details] updated Patch.

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)

Also, it's likely possible to make a regression test for this fix.

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.

> 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.

Created attachment 106331 [details] Patch

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 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

(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 :)

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$ ===============================================================================

(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.

Created attachment 107149 [details] Updated patch

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

Created attachment 107160 [details] Updated patch

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.

(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 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 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.

Created attachment 107644 [details] Updated Patch

Comment on attachment 107644 [details] Updated Patch Clearing flags on attachment: 107644 Committed r95381: <http://trac.webkit.org/changeset/95381>

All reviewed patches have been landed. Closing bug.