Closed Bug 943935 Opened 11 years ago Closed 10 years ago

<button>s shouldn’t trigger the activation behaviour of parent <label>s

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 229925

People

(Reporter: robin, Unassigned)

References

()

Details

(Keywords: testcase)

Attachments

(5 files, 1 obsolete file)

Steps to reproduce:
1) Load testcase from URL field
2) Click the button

Actual results:
Checkbox is checked

Expected results:
Checkbox shouldn’t check. The spec says:

The label element's exact default presentation and behavior, in particular what its activation behavior might be, if anything, should match the platform's label behavior. The activation behavior 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.
http://www.w3.org/html/wg/drafts/html/master/forms.html#the-label-element

A <button> element is interactive content: http://www.w3.org/html/wg/drafts/html/master/dom.html#interactive-content-0
Attached patch wip (obsolete) — Splinter Review
This prevents the button click from changing the checkbox at least.
It still shows a grey check-mark in the mouse-down state though.
And IsInteractive() is obviously wrong, it needs to codify HTML5 "interactive content"
(and possibly some XUL elements too).  Boris, do you know if we have code for that
somewhere?

http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#interactive-content
Flags: needinfo?(bzbarsky)
Keywords: testcase
OS: Mac OS X → All
Hardware: x86 → All
The grey check-mark in the mouse-down state is due to :active styling on the checkbox, presumably.
I don't know of such code offhand, sorry.
Flags: needinfo?(bzbarsky)
I find the HTML5 definition a bit weak ... why is <a> (without href) interactive for example?

I found IsElementClickable() which looks a lot more realistic:
http://mxr.mozilla.org/mozilla-central/source/layout/base/PositionedEventTargeting.cpp#156

How about we take the guts out of that and put it in nsLayoutUtils and use that instead?
(i.e. leave the for-loop, move the rest to nsLayoutUtils::IsInteractiveContent() or something)
(In reply to Boris Zbarsky [:bz] from comment #4)
> The grey check-mark in the mouse-down state is due to :active styling on the
> checkbox, presumably.

Yeah, we forward state changes in nsEventStateManager::UpdateAncestorState,
so after I fixed that the test seems to work.  Let's see how much breaks:
https://tbpl.mozilla.org/?tree=Try&rev=12bdf6dda31e
Assignee: nobody → matspal
Attachment #8339528 - Attachment is obsolete: true
Attached file Testcase
Getting the latter part here was tricky and something I missed in my
first Try push (if you want to compare).  Currently the code only
deals with !HasState and setting the state in that case, but we also
need to deal with HasState -> clearing the state.  This happens when
hovering between a link and plain text inside a label.  They should
clear/set the hover state on the labeled element respectively.

Try push with this set of patches:
https://tbpl.mozilla.org/?tree=Try&rev=8ace469fc440

(I would guess this also fixes bug 163912, but I can't tell how to
reproduce that bug.  The comments doesn't match what happens in
the tests anymore.)
Attachment #8339727 - Flags: review?(bzbarsky)
Comment on attachment 8339716 [details] [diff] [review]
Move the guts of IsElementClickable to a separate function - nsContentUtils::IsInteractiveContent.

So this is nothing like the spec's definition of interactive content, right?  That's just a tag+attribute whitelist, while this implementation considers event handlers too....  Specifically, http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#interactive-content-0

Do we need to raise a spec issue, or change this code, or something else?
Attachment #8339716 - Flags: review?(bzbarsky) → review-
Comment on attachment 8339718 [details] [diff] [review]
Rename EventTargetIn to ShouldForwardEventForTarget (reversing the logic) and make it return false for IsInteractiveContent descendants of the label.

This is again not matching the spec.  Per spec we should stop if we say any interactive content on the path from the target to the label.  So in this case:

  <label><a href="foo"><span>text</span></a></label>

clicking on the text should not forward the click, right?
Attachment #8339718 - Flags: review?(bzbarsky) → review-
Comment on attachment 8339721 [details] [diff] [review]
Declare a local loop variable since we need the original aStartNode value in the next part.

r=me
Attachment #8339721 - Flags: review?(bzbarsky) → review+
Comment on attachment 8339727 [details] [diff] [review]
Don't forward state changes to the labeled element if the label's ShouldForwardEventForTarget returns false.

Please just "using mozilla::dom".

>+      if (labelTarget && labelTarget != aStartNode) {

I believe this is wrong.  If I understand the intent of the != aStartNode test correctly (and comments would go a long way here), the idea is that we don't want to remove aState from aStartNode just because label happens to not forward to it.

But that's true not just for aStartNode but also for all its ancestors.

In general, in this branch aAddState is true.  We should not be removing any states; that already happened earlier.  All we need to do is make sure to add them to things that they need adding to.  So perhaps something like this:

  if (labelTarget && !labelTarget->State().HasState(aState) &&
      label->ShouldForwardEventForTarget(aStartNode)) {
    DoStateChange(labelTarget, aState, true);
  }

But also, please raise a spec issue about this.  In particular, the spec talks about doing this for :hover at http://www.whatwg.org/specs/web-apps/current-work/multipage/selectors.html#selector-hover but with no reference to the interactive element behavior bits and doesn't talk about the :active behavior we want at http://www.whatwg.org/specs/web-apps/current-work/multipage/selectors.html#selector-active at all.  It should probably do both.
Attachment #8339727 - Flags: review?(bzbarsky) → review-
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: mats → nobody
The fix hasn’t made it into a release build yet, but this might be fixed by bug #229925 .
Yeah, looks fairly likely.  You could check a nightly to make sure?
Depends on: 229925
Latest Nightly and the testcase works, so yeah, fixed by bug #229925.
Great, thank you for checking that!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: