Closed Bug 1141455 Opened 5 years ago Closed 5 years ago

Checkbox not clickable when parent element has tabindex

Categories

(Core :: General, defect)

38 Branch
x86
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla39
Tracking Status
firefox37 --- unaffected
firefox38 + verified
firefox39 + verified

People

(Reporter: kemenaran, Assigned: arai)

References

()

Details

(Keywords: regression, Whiteboard: [bugday-20150610])

Attachments

(2 files)

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).
Status: UNCONFIRMED → NEW
Component: Untriaged → CSS Parsing and Computation
Ever confirmed: true
Product: Firefox → Core
[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.
Blocks: 1135374
Component: CSS Parsing and Computation → General
Flags: needinfo?(catalin.varga)
Tracking for 38 and 39 since this looks like a recent web compatibility regression.
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?
Flags: needinfo?(arai.unmht) → needinfo?(bugs)
Removing the needinfo as the regression window was provided by Alice in comment 3
Flags: needinfo?(catalin.varga)
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.
Flags: needinfo?(bugs)
(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.
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
Attachment #8578379 - Flags: review?(bugs)
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.
Attachment #8578379 - Flags: review?(bugs) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/39a861ac2bb6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Tooru, could you fill the uplift request to 38? thanks
Flags: needinfo?(arai.unmht)
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
Flags: needinfo?(arai.unmht)
Attachment #8581285 - Flags: review+
Attachment #8581285 - Flags: approval-mozilla-aurora?
Attachment #8581285 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee: nobody → arai.unmht
QA Whiteboard: [good first verify]
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)
QA Whiteboard: [good first verify] → [good first verify][bugday-20150610]
Whiteboard: [bugday-20150610]
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.