720130 - tab key should not focus <object> nodes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Pascal Chevrel:pascalc]

This is a bug that was reported and demoed to me in the accessibility workshop we have today in the Mozilla Paris office. Blind people use tabbing a lot to move from link to link in a page but in the last year, there was  a regression in functionnality in Firefox in that if the page has a video in it (flash video in an object tag), the video gets focused with pressing tab and the tab key then becomes a dead key, you can no longer use the keyboard to navigate in the page.

This is something that used to work in older 3.x versions and it works in all browsers except Firefox.

I used mozregression to find when this functional regression happened and here is the result:

Last good nightly: 2010-04-29
First bad nightly: 2010-04-30

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=797e4ee41b51&tochange=c9212eb6175b

The url I used for my test is this one:
http://tentatives-accessibles.eu/191-court-reportage-sur-l-audiodescription


In the pushlog above, one bug looks like it could be the source of this regression:

b8c6a4f81a58	Neil Deakin — Bug 554635, change default tabindex on object elements to 0, so that child documents can be focused

Digging into bugzilla, it looks like Bug 596350 was a followup to bug 554635 to fix this regression, but it doesn't look like it had a real effect on the pages I tried.

CCing Mounir and Neil who were working on these bugs

Comment 1

[Anthony Ricaud (:rik)]

Related, bug 93149

Comment 2

[Neil Deakin]

When I try the test link above I can tab into the plugin and within the individual buttons within the flash plugin. I just can't escape again, so isn't this just the same as bug 93149?

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to Neil Deakin from comment #2)
> When I try the test link above I can tab into the plugin and within the
> individual buttons within the flash plugin. I just can't escape again, so
> isn't this just the same as bug 93149?

They are indeed similar but I think the difference is that since bug 554635, users can give focus to a plugin so they will easily hit bug 93149. Before, the browser had to give the focus to the plugin or the user had to explicitly give the focus to the plugin to hit that bug.

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Try

So, I do not remember why I did allow HTMLObjectElements with plugins to get focus while tabbing. I guess it wasn't useful to fix the issues we saw but I might have missed something.

Does someone see why that patch wouldn't be okay?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 592097 [details] [diff] [review]
Try

Yeah, this seems reasonable.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4)
> Created attachment 592097 [details] [diff] [review]
> Try
> 
> So, I do not remember why I did allow HTMLObjectElements with plugins to get
> focus while tabbing.
Because it is the right thing to do?

We should really fix bug 93149 somehow.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 592097 [details] [diff] [review]
Try

But we need to fix this somehow for now. So doing this is ok, IMO.

Comment 8

[Mounir Lamouri (:mounir)]

It seems like bug 491141 would be needed to fix bug 93149 but before this happening we need to implement that in Firefox and have plugins using the new NPAPI capabilities.
Given that the a11y issue seems important here, we should probably just fix bug this bug.

Comment 9

[Josh Aas]

I like this idea. Thanks for looking into it.

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

I believe this patch should fix the issue and still allow .focus() to work.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 594159 [details] [diff] [review]
Patch v1

 
>   // This method doesn't call nsGenericHTMLFormElement intentionally.
>   // TODO: It should probably be changed when bug 597242 will be fixed.
>   if (Type() == eType_Plugin || IsEditableRoot() ||
>       (Type() == eType_Document && nsContentUtils::IsSubDocumentTabbable(this))) {
>     // Has plugin content: let the plugin decide what to do in terms of
>     // internal focus from mouse clicks
>     if (aTabIndex) {
>-      GetIntAttr(nsGkAtoms::tabindex, 0, aTabIndex);
>+      GetTabIndex(aTabIndex);
>     }
Is this what other browsers do? Using tabindex in plugin <object> makes it tabbable?


>+<div id="content">
>+  <input>
>+  <object type="application/x-test"></object>
Do we have a test for 
<object type="application/x-test" tabindex="0"></object> ?

Comment 12

[Mounir Lamouri (:mounir)]

(In reply to Olli Pettay [:smaug] from comment #11)
> Comment on attachment 594159 [details] [diff] [review]
> Patch v1
> 
>  
> >   // This method doesn't call nsGenericHTMLFormElement intentionally.
> >   // TODO: It should probably be changed when bug 597242 will be fixed.
> >   if (Type() == eType_Plugin || IsEditableRoot() ||
> >       (Type() == eType_Document && nsContentUtils::IsSubDocumentTabbable(this))) {
> >     // Has plugin content: let the plugin decide what to do in terms of
> >     // internal focus from mouse clicks
> >     if (aTabIndex) {
> >-      GetIntAttr(nsGkAtoms::tabindex, 0, aTabIndex);
> >+      GetTabIndex(aTabIndex);
> >     }
> Is this what other browsers do? Using tabindex in plugin <object> makes it
> tabbable?

This is what Firefox 3.6 was doing. Chrome doesn't focus the <object> but Opera does it (but you can unfocus it by pressing TAB).
IMO, we should not ignore the tabindex value and if an author put a tabindex on an <object> that means the object should be part of the navigation.

However, we can discuss that but I think that should be done in another bug because we were doing that in Firefox 3.6 and we are doing that in the current version of Firefox so not doing that anymore might have different consequences than what the patch is actually doing.

> >+<div id="content">
> >+  <input>
> >+  <object type="application/x-test"></object>
> Do we have a test for 
> <object type="application/x-test" tabindex="0"></object> ?

Maybe. I can add tests for that if you want.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 594159 [details] [diff] [review]
Patch v1

Ok, and please add the tabindex=0 test

Comment 14

[Mounir Lamouri (:mounir)]

Comment on attachment 594159 [details] [diff] [review]
Patch v1

Pushed in mozilla-inbound with the requested changes.

Comment 15

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/5a2ac7c5851e