1141455 - Checkbox not clickable when parent element has tabindex

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[Pierre de La Morinerie]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150309004203

Steps to reproduce:

1. Have an HTML structure of label -> div with tabindex -> checkbox-input
2. Click inside the div with tabindex


Actual results:

The checkbox is not selected.


Expected results:

The checkbox should be selected.

The actual behavior is:

* clicking on the label area toggles the checkbox (correctly)
* clicking on the checkbox itself toggles the checkbox (correctly)
* BUT clicking on the div with `tabindex` doesn't toggle the checkbox (incorrectly)

Here is a JSBin that demonstrates the issue: http://jsbin.com/budisehuyo/3/

This issue is present in Firefox 38a2 and 39a1 (but not before; it's a regression). It appears on www.capitainetrain.com (see 1135374).

Comment 1

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Web compat regression

Uh... This has nothing to do with CSS.  There's no CSS in the testcase at all.

Presumably the issue is that the click is focusing the scrollable area itself.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 38 and 39 since this looks like a recent web compatibility regression.

Comment 3

[Alice0775 White]

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=675a7c4287a5&tochange=e8981c280f7b

Regressed by: Bug 229925

Comment 4

[Tooru Fujisawa [:arai]]

I think current behavior matches to what the spec says.

https://html.spec.whatwg.org/multipage/forms.html#the-label-element:the-label-element-10
> The activation behaviour of a label element for events targeted at interactive
> content descendants of a label element, and any descendants of those
> interactive content descendants, must be to do nothing.

https://html.spec.whatwg.org/multipage/dom.html#interactive-content-2
> Interactive content is content that is specifically intended for user
> interaction.
> The tabindex attribute can also make any element into interactive content.

tabindex makes span element an interactive content, and label's activation behaviour must be to do nothing, instead of clicking on checkbox, for the event on the span element.

So, this should be a spec issue.

To make it work same as before, removing `HasAttr(kNameSpaceID_None, nsGkAtoms::tabindex)` condition from nsGenericHTMLElement::IsInteractiveHTMLContent will solve.
https://hg.mozilla.org/mozilla-central/file/0190a1d17294/dom/html/nsGenericHTMLElement.cpp#l1801
> bool
> nsGenericHTMLElement::IsInteractiveHTMLContent() const
> {
>   return IsAnyOfHTMLElements(nsGkAtoms::details, nsGkAtoms::embed,
>                              nsGkAtoms::keygen) ||
>          HasAttr(kNameSpaceID_None, nsGkAtoms::tabindex);
> }

smaug, what's the appropriate solution for this case?

Comment 5

[Catalin Varga [QA][:VarCat]]

Removing the needinfo as the regression window was provided by Alice in comment 3

Comment 6

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

What do other browsers do here? Sounds like we need to follow their behavior and get the spec fixed.
(Please file a spec bug.)
Perhaps removing the tabindex check is the right thing here. But then, having tabindex makes element interactive... clearly the spec needs clarification.
For now, perhaps we could add a param to IsInteractiveHTMLContent to tell whether or not check
tabindex attribute.

Comment 7

[Tooru Fujisawa [:arai]]

(In reply to Olli Pettay [:smaug] from comment #6)
> What do other browsers do here? Sounds like we need to follow their behavior
> and get the spec fixed.

Tested on following, and clicking on span with tabindex turns on/off the checkbox on all of them, so now only Firefox 38.0a2 and 39.0a1 behave differently.

Windows 7
  IE 11.09600.17633

Mac OS X 10.9.5
  Google Chrome 41.0.2272.89 (64-bit)
  Google Chrome Canary 43.0.2331.3 canary (64-bit)
  Safari 7.1.3 (9537.85.12.18)
  WebKit r181478


> (Please file a spec bug.)

Filed here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28212


> For now, perhaps we could add a param to IsInteractiveHTMLContent to tell
> whether or not check
> tabindex attribute.

I'll make a patch.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Do not treat an element with tabindex as an interactive content in label.

Added ignoreTabindex parameter to IsInteractiveHTMLContent, and pass true from InInteractiveHTMLContent in HTMLLabelElement.cpp.

Almost green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fced6d072b4

Comment 9

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

Comment on attachment 8578379 [details] [diff] [review]
Do not treat an element with tabindex as an interactive content in label.

s/ignoreTabindex/aIgnoreTabindex/

I realized the current handling of tabindex doesn't even follow the spec, since
any element with tabindex should make it interactive.
So, HTMLObjectElement::IsInteractiveHTMLContent for example should call its parent class to see if tabindex is used.
But that fix can be done in a different bug, since it doesn't affect the fix.

Comment 10

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8578379 [details] [diff] [review]
> Do not treat an element with tabindex as an interactive content in label.
> 
> s/ignoreTabindex/aIgnoreTabindex/

fixed locally :)
I'll push it to m-i after tree open.

> I realized the current handling of tabindex doesn't even follow the spec,
> since
> any element with tabindex should make it interactive.
> So, HTMLObjectElement::IsInteractiveHTMLContent for example should call its
> parent class to see if tabindex is used.
> But that fix can be done in a different bug, since it doesn't affect the fix.

Oops, thanks for pointing it out, filed as bug 1144322.

Comment 11

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39a861ac2bb6

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/39a861ac2bb6

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Tooru, could you fill the uplift request to 38? thanks

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-aurora) Do not treat an element with tabindex as an interactive content in label. r=smaug

Thank you for the reminder :)
Backported to mozilla-aurora. no major changes.

Approval Request Comment
[Feature/regressing bug #]: bug 229925
[User impact if declined]: Label element doesn't work as expected, and becomes non-clickable in some case.
[Describe test coverage new/current, TreeHerder]: both in test suite (dom/html/test/forms/test_interactive_content_in_label.html)
[Risks and why]: Low. it disables a part of changes in bug 229925. so it just becomes previous behavior.
[String/UUID change made/needed]: no

Comment 15

[Tooru Fujisawa [:arai]]

Thank you again!

https://hg.mozilla.org/releases/mozilla-aurora/rev/325daf43b8f1

Comment 16

[Ashickur Rahman]

Successfully reproduce the bug of Aurora 38a2 (build ID: 20150310004228) nigthly 39a1 (build ID: 20150310030235) .

Fixed works for me on Firefox 39.0 Linux 64 bit (build ID : 20150604162752 , User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0)

Comment 17

[Ashickur Rahman]

Fixed works for me on Firefox 38.0.5 Windows (build ID : 20150525141253 , Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0)