Bug 67190 – A Media Element ie: <Audio> or <video>, without tabindex cannot be selected with keyboard (TAB Key).

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

[Deepak Sherveghar]

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

[Deepak Sherveghar]

Created attachment 105605 [details]
Proposed Patch.

[Deepak Sherveghar]

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.

[Deepak Sherveghar]

Created attachment 105607 [details]
updated Patch.

[Eric Carlson]

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)

[Alexey Proskuryakov]

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

[Ian 'Hixie' Hickson]

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.

[Antonio Gomes]

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

[Deepak Sherveghar]

Created attachment 106331 [details]
Patch

[Antonio Gomes]

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

[WebKit Review Bot]

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

[Deepak Sherveghar]

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

[Deepak Sherveghar]

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

[Hayato Ito]

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

[Deepak Sherveghar]

Created attachment 107149 [details]
Updated patch

[WebKit Review Bot]

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

[Deepak Sherveghar]

Created attachment 107160 [details]
Updated patch

[Antonio Gomes]

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.

[Deepak Sherveghar]

(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

[Ryosuke Niwa]

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?

[Antonio Gomes]

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.

[Deepak Sherveghar]

Created attachment 107644 [details]
Updated Patch

[WebKit Review Bot]

Comment on attachment 107644 [details]
Updated Patch

Clearing flags on attachment: 107644

Committed r95381: <http://trac.webkit.org/changeset/95381>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.