1167862 - Interactive content inside <label> changes the appearance of associated <input type="checkbox"> as if activating it

Status: NEW

(Core :: Layout: Form Controls, defect)

Description

[Gingerbread Man]

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150523030206

Steps to reproduce:
1. Open the link in the URL field.
2. Click “Click here”.

Actual results:
You can see the appearance of the checkbox change as if you're clicking it. Particularly confusing in Linux, where it gets a checkmark that disappears once you release the mouse button.

Expected results:
The appearance of the checkbox shouldn't change, same as in Internet Explorer 11.

Comment 1

[Tooru Fujisawa [:arai]]

So, this will be a spec issue.

https://html.spec.whatwg.org/multipage/scripting.html#selector-active
> If the element is the labeled control of a label element that is currently matching :active
>   The element is being activated.

Labeled control should be activated only if the label's activation behavior is not "do nothing", since the element won't be actually activated on click.

https://html.spec.whatwg.org/multipage/scripting.html#selector-hover
> Consider in particular a fragment such as:
> 
> <p> <label for=c> <input id=a> </label> <span id=b> <input id=c> </span> </p>
> 
> If the user designates the element with ID "a" with their pointing device,
> then the p element (and all its ancestors not shown in the snippet above),
> the label element, the element with ID "a", and the element with ID "c" will
> match the :hover pseudo-class. The element with ID "a" matches it from
> condition 1, the label and p elements match it because of condition 2 (one of
> their descendants is designated), and the element with ID "c" matches it
> through condition 3 (its label element matches :hover). However, the element
> with ID "b" does not match :hover: its descendant is not designated, even
> though it matches :hover.

ID "c" shouldn't match :hover, with same reason as :active.

I'll post a patch (almost ready, waiting for try run) and file a spec bug shortly.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Do not set label target state if the activation behavior is do nothing.

The differences between current spec/impl and the expected behavior are following.

 1. label's target shouldn't match :hover/:active when cursor is inside other interactive content
  e.g.
    <label for="foo">
      <a id="X" href="...">X</a>
    </label>
    when cursor is on #X, #foo should not match :hover

 2. from 1, at most one label target matches :hover/:active at a certain moment
  e.g.
    <label for="foo">
      <label id="X" for="bar">X</label>
    </label>
    when cursor is on #X, only #bar matches :hover

 3. (impl specific) affected label might be ancestor of common ancestor of current and previous hovered nodes
  e.g.
    <label for="foo">
      <span id="X">
        <span id="Y">Y</span>
        <a id="Z"  href="...">Z</a>
      </span>
    </label>
    when cursor moved from #Y to #Z, common ancestor is #X, and we're currently
    checking label and changing the state up to #X, but #foo's state should be changed

From 3, we should use different loop than UpdateAncestorState.
From 1 and 2, we can quit the loop when interactive content (including label) is found, and change the state only if the first interactive content is a label and it has target.

So, I removed the code related to label target from UpdateAncestorState, and added the code which updates the state of nearest label's target, only if no other interactive content found between current node and the label.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3f04de3c074

Comment 3

[Tooru Fujisawa [:arai]]

reported a spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28700

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 8610595 [details] [diff] [review]
Do not set label target state if the activation behavior is do nothing.

This doesn't seem right to me.  I believe the behavior in Chrome is
the correct one for the given testcase.  That is, it should behave
the same as if <a> is replaced with <x>:
data:text/html,<label for="a"><x>Click here</x></label><input id="a" type="checkbox">

As far as I know, there is nothing special about a plain <a> element
(without a href) so I don't see why the proposed change is correct.
Clicking on "Click here" should toggle the checkbox.

Comment 5

[Tooru Fujisawa [:arai]]

(In reply to Mats Palmgren (:mats) from comment #4)
> Comment on attachment 8610595 [details] [diff] [review]
> Do not set label target state if the activation behavior is do nothing.
> 
> This doesn't seem right to me.  I believe the behavior in Chrome is
> the correct one for the given testcase.  That is, it should behave
> the same as if <a> is replaced with <x>:
> data:text/html,<label for="a"><x>Click here</x></label><input id="a"
> type="checkbox">
> 
> As far as I know, there is nothing special about a plain <a> element
> (without a href) so I don't see why the proposed change is correct.
> Clicking on "Click here" should toggle the checkbox.

This bug is originally separated from bug 1167816, and the behavior related to <a> element without href attribute will be addressed there.  I'd like to work on more general behavior related to label and interactive content here.

Comment 6

[Tooru Fujisawa [:arai]]

So, using <input> in the testcase will be appropriate :)

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 8610595 [details] [diff] [review]
Do not set label target state if the activation behavior is do nothing.

Oh, OK.  Perhaps next time you should explain that you're fixing a completely
different problem than the one reported, to avoid confusion.  :-)

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 8610595 [details] [diff] [review]
Do not set label target state if the activation behavior is do nothing.

Oops, I misread the suggested list, sorry!

Comment 9

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

So I can't reproduce the bug as described in comment 0, on linux.
And on Windows Chrome and Firefox behave the same.

I must be missing something here.

Comment 10

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

oh, comment 7... What is this bug about then?

Comment 11

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

Comment on attachment 8610595 [details] [diff] [review]
Do not set label target state if the activation behavior is do nothing.

Since I don't know what bug we're trying to fix here, clearing the review request for now. Please explain and re-ask review.

Comment 12

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

Note, there was a recent regression about :active handling, and I think the patch landed very recently.

Comment 13

[Tooru Fujisawa [:arai]]

Comment on attachment 8610595 [details] [diff] [review]
Do not set label target state if the activation behavior is do nothing.

sorry for my unclear explanation.
This bug is about handling of :hover and :active pseudo class for label target, when cursor is on interactive content inside the label.

The original problem (bug 1167816) is about the behavior of <a> inside <label>, with following HTML:
  data:text/html,<label for="a"><a>Click here</a></label><input id="a" type="checkbox">
There were two issues.
  1. clicking the <a> doesn't check/uncheck the checkbox
  2. clicking the <a> changes checkbox's appearance (because of :active pseudo class), despite of 1

For 1, we need to exclude <a> without href attribute from interactive content, I'd like to fix it in bug 1167816.  This behavior was introduced by bug 229925, where I made the label the spec compliant.

For 2, we need to avoid applying :hover/:active pseudo class to label target if label's activation behavior is "do nothing" (currently we're applying :hover/:active to label target whenever label matches :hover/:active), and I'd like to fix it in this bug.

Both are spec bugs.  They're not recent regressions from other bugs, I guess.

Comment 14

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

but 2. is what I'm seeing also in other browsers. Is 2. a bug or a feature or a spec bug?

Comment 15

[Tooru Fujisawa [:arai]]

IE11 doesn't apply :hover/:active in that case. there exist some glitchy behavior though (:hover is not applied again when cursor moves out from interactive content).

Also, I don't think current behavior is intuitive.  iirc, firefox's current behavior matches to the spec, so I think this is a spec bug (am I using correct term?)

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: demo for labeled target with :hover and :active

this demo applies outline to :hover and :active.
  dotted outline: no class
  solid outline: :hover
  double outline: :active

steps
  1. move cursor to label element
  2. click
  3. move cursor to a element
  4. click
  5. move curosr to label element
  6. click
  7. move cursor out of label element

results:
  Firefox
    1. checkbox has solid outline (:hover)
    2. checkbox has double outline (:active)
    3. checkbox has solid outline (:hover)
    4. checkbox has double outline (:active)
    5. checkbox has solid outline (:hover)
    6. checkbox has double outline (:active)
    7. checkbox has dotted outline (no class)

  IE11
    1. checkbox has solid outline (:hover)
    2. checkbox has double outline (:active)
    3. checkbox has dotted outline (no class)
    4. checkbox has dotted outline (no class)
    5. checkbox has dotted outline (no class)
    6. checkbox has double outline (:active)
    7. checkbox has dotted outline (no class)

IE11's results for 3. and 4. are the behavior what I want to implement in firefox.
IE11's result for 5. seems to be their bug (not yet confirmed on edge).

Comment 17

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

Chrome and Safari seems to have the same behavior as Firefox in 3. and Safari also in 4.
(oddly Chrome doesn't show double outline in 4. )

And bug 229925 didn't change our behavior in 3. or 4. So why you want to change the behavior to be against the current spec and what we and webkit have?

Comment 18

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

Comment on attachment 8610595 [details] [diff] [review]
Do not set label target state if the activation behavior is do nothing.

So, it is not clear to me why we'd want to move from
spec/old-gecko/webkit/blink behavior to trident behavior.

Please re-ask review if I've misunderstood something here :)

Comment 19

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

(and don't care about the message about reviewing overload if you think the patch does have the behavior we should have. Just explain why, and reask review.)

Comment 20

[Tooru Fujisawa [:arai]]

Yeah, bug 229925 didn't touch selector.  Just noted that behavior in bug 229925 comment #26, but it was out of scope for that bug.

Then, the reason why I'd like to change the behavior is that the behavior is misleading.  If cursor is on interactive content inside the label, the label's activation behavior is "do nothing".  So, clicking the interactive content doesn't actually activate the label target, such as checking/unchecking the checkbox, pushing button, etc.  Applying :hover and especially :active will mislead users to think that the label target is being somehow activated.  In bug 1167816 comment #0, it was interpreted as "the checkbox quickly toggles the checked attribute and restores it to what it was before clicking".  Don't you think this is a problem?

Comment 21

[Tooru Fujisawa [:arai]]

I should've noted that the demo doesn't test the behavior on chrome, since chrome doesn't classify <a> without href as interactive content (the result is same tho).

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: demo with <a> with href attribute (href="#")

this will be better for testing on all browsers

Comment 23

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

I think we should change gecko's behavior only after spec change in this case.

Comment 24

[Tooru Fujisawa [:arai]]

Okay, thank you for your feedback :)

Comment 25

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)