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

Status: RESOLVED DUPLICATE of bug 229925

(Core :: Layout: Form Controls, defect)

Description

[Robin Whittleton]

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

Comment 1

[Mats Palmgren (inactive)]

Attachment: wip

This prevents the button click from changing the checkbox at least.
It still shows a grey check-mark in the mouse-down state though.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note bug 163912.

Comment 3

[Mats Palmgren (inactive)]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

The grey check-mark in the mouse-down state is due to :active styling on the checkbox, presumably.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I don't know of such code offhand, sorry.

Comment 6

[Mats Palmgren (inactive)]

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)

Comment 7

[Mats Palmgren (inactive)]

(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

Comment 8

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 9

[Mats Palmgren (inactive)]

Attachment: Move the guts of IsElementClickable to a separate function - nsContentUtils::IsInteractiveContent.

Comment 10

[Mats Palmgren (inactive)]

Attachment: Rename EventTargetIn to ShouldForwardEventForTarget (reversing the logic) and make it return false for IsInteractiveContent descendants of the label.

Comment 11

[Mats Palmgren (inactive)]

Attachment: Declare a local loop variable since we need the original aStartNode value in the next part.

Comment 12

[Mats Palmgren (inactive)]

Attachment: Don't forward state changes to the labeled element if the label's ShouldForwardEventForTarget returns false.

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.)

Comment 13

[Boris Zbarsky [: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?

Comment 14

[Boris Zbarsky [:bzbarsky]]

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?

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[Robin Whittleton]

The fix hasn’t made it into a release build yet, but this might be fixed by bug #229925 .

Comment 18

[Boris Zbarsky [:bzbarsky]]

Yeah, looks fairly likely.  You could check a nightly to make sure?

Comment 19

[Robin Whittleton]

Latest Nightly and the testcase works, so yeah, fixed by bug #229925.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Great, thank you for checking that!