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

NEW
Assigned to

Status

()

defect
--
minor
4 years ago
Last year

People

(Reporter: gingerbread_man, Assigned: arai)

Tracking

({parity-ie, reproducible, testcase})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected)

Details

()

Attachments

(3 attachments)

Reporter

Description

4 years ago
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.
Assignee

Comment 1

4 years ago
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.
Assignee: nobody → arai.unmht
Assignee

Comment 2

4 years ago
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
Attachment #8610595 - Flags: review?(smontagu)
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.
Attachment #8610595 - Flags: feedback-
Assignee

Comment 5

4 years ago
(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.
See Also: → 1167816
Assignee

Comment 6

4 years ago
So, using <input> in the testcase will be appropriate :)
Summary: <a> child of <label> changes the appearance of associated <input type="checkbox"> as if activating it → Interactive content inside <label> changes the appearance of associated <input type="checkbox"> as if activating it
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.  :-)
Attachment #8610595 - Flags: feedback-
Assignee

Comment 8

4 years ago
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!
Attachment #8610595 - Flags: review?(smontagu) → review?(bugs)
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.
oh, comment 7... What is this bug about then?
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.
Flags: needinfo?(arai.unmht)
Attachment #8610595 - Flags: review?(bugs)
Note, there was a recent regression about :active handling, and I think the patch landed very recently.
Assignee

Comment 13

4 years ago
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.
Flags: needinfo?(arai.unmht)
Attachment #8610595 - Flags: review?(bugs)
but 2. is what I'm seeing also in other browsers. Is 2. a bug or a feature or a spec bug?
Flags: needinfo?(arai.unmht)
Assignee

Comment 15

4 years ago
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?)
Flags: needinfo?(arai.unmht)
Assignee

Comment 16

4 years ago
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).
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 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 :)
Attachment #8610595 - Flags: review?(bugs) → review-
(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.)
Assignee

Comment 20

4 years ago
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?
Flags: needinfo?(bugs)
Assignee

Comment 21

4 years ago
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).
Assignee

Comment 22

4 years ago
this will be better for testing on all browsers
I think we should change gecko's behavior only after spec change in this case.
Flags: needinfo?(bugs)
Assignee

Comment 24

4 years ago
Okay, thank you for your feedback :)
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-ie
Whiteboard: [parity-ie]
You need to log in before you can comment on or make changes to this bug.