Bug 39620 – Clicks inside button elements are sometimes discarded when the mouse moves

• Component: Forms
• Status: REOPENED

Created attachment 56929 [details]
image of a button with inner text box

When a click originates inside one element of a button (e.g., the button text) and terminates outside of that element (e.g., another element, or just the button box) the onClick() method isn't triggered.

This is reproducible on websites like facebook.  You can reproduce by clicking inside the text box, holding the mouse down, and then releasing the mouse button outside of the text box, but still inside the button.  While the facebook buttons are styled such that this doesn't happen by accident all that often, buttons with slightly more padding cause this issue frequently.

Both safari and chrome exhibit this behavior, but firefox does not.

Attached is a button from facebook that shows the inner text box inside the larger button box.  A click that Goes from inside-to-outside will not register.

[Alexey Proskuryakov]

Someone needs to grab this bit of HTML/CSS code to investigate.

Created attachment 88775 [details]
Simple Page which demostrates this bug.

This pages shows the bug. It works correctly in Firefox 3.6.

I hit this bug, too. I'm using QtWebKit shipped with Qt 4.7.2 on Windows XP. This bug also affects Safari 5.0.4 on Windows XP.

I stepped through the QtWebKit code. No click event is generated in webkit\WebCore\page\EventHandler.cpp because mev.targetNode() != m_clickNode in the function 
bool EventHandler::handleMouseReleaseEvent(const PlatformMouseEvent& mouseEvent)

[Ryosuke Niwa]

Confirmed on Nightly WebKit. The bug does not reproduce on Firefox 4.

[Kent Tamura]

IE8 behaves as same as Firefox.
Ok, we should fix this.

[Ryosuke Niwa]

"5.2.3.2. Mouse Event Order" (http://www.w3.org/TR/DOM-Level-3-Events/#events-mouseevent-event-order) has the following statement:

in general should fire click anddblclick events when the event target of the associated mousedown and mouseup events is the same element with no mouseout or mouseleave events intervening, and should not fire click and dblclick events when the event target of the associated mousedown and mouseup events is different.

So current behavior of WebKit follows the spec. Firefox avoids this bug by firing all mousedown, mouseup, and click on button element and Internet Explorer always fires click regardless of whether mouseup/mousedown's targets match each other.

[Alexey Proskuryakov]

User experience is definitely wrong here, so this is something that needs to be fixed one way or another.

I guess the question is whether text inside a button is a separate element from the button, or part of the button.  I think common sense would say that any text in a button is logically part of the button itself.

[Ryosuke Niwa]

Per discussion on IRC, I sent an email to www-dom: http://lists.w3.org/Archives/Public/www-dom/2011AprJun/0025.html

We should wait fixing this bug until some consensus is reached there.

[Ryosuke Niwa]

I'm going to assign the bug to myself at least until I get some response on www-dom.

[Ryosuke Niwa]

Created attachment 93116 [details]
work in progress

In this patch, I'm dispatching click event on the least common ancestor of clickNode and the target node for mouseup event.  I'm not sure this is the correct approach given that IE and FF both special cases button element.

[Dimitri Glazkov (Google)]

I think we can reason about this using shadow DOM as a primitive:

1) Inside shadow DOM, mouseover/out events only escape the shadow DOM boundary if the relatedTarget of an event is outside of the shadow DOM boundary;

2) if we pretend that a button is a shadow DOM boundary, then the user should not hear mouseovers/outs when moving the mouse inside the button.

3) Since the events are retargeted to the outside of the shadow boundary, the effective target of the mouseup and mousedown events is always going to be the button element.

4) If so, the 5.2.3.2. section that Niwa-san mentioned makes perfect sense, relative to the outside of the shadow DOM boundary.

From this, it appears that click and dblclick events are relative in terms of the shadow DOM:

* Inside of the shadow DOM boundaries, the mousedown, mouseover, mouseout, and mouseup are all separate events.

* Outside of the shadow DOM boundary, they appear as part of a click/dblclick event sequence.

As an aside, this is a good indication that the current click/dblclick logic should probably move down from EventHandler, and into EventDispatcher, since only EventDispatcher is aware of the shadow DOM boundaries.

[Ryosuke Niwa]

(In reply to comment #12)
> 1) Inside shadow DOM, mouseover/out events only escape the shadow DOM boundary if the relatedTarget of an event is outside of the shadow DOM boundary;

What is a related target?

> 2) if we pretend that a button is a shadow DOM boundary, then the user should not hear mouseovers/outs when moving the mouse inside the button.
> 
> 3) Since the events are retargeted to the outside of the shadow boundary, the effective target of the mouseup and mousedown events is always going to be the button element.
> 
> 4) If so, the 5.2.3.2. section that Niwa-san mentioned makes perfect sense, relative to the outside of the shadow DOM boundary.

However, a button doesn't have a shadow DOM. So I have a problem with the premise in point 2.

[Dimitri Glazkov (Google)]

(In reply to comment #13)
> (In reply to comment #12)
> > 1) Inside shadow DOM, mouseover/out events only escape the shadow DOM boundary if the relatedTarget of an event is outside of the shadow DOM boundary;
> 
> What is a related target?

https://developer.mozilla.org/en/DOM/event.relatedTarget

> 
> > 2) if we pretend that a button is a shadow DOM boundary, then the user should not hear mouseovers/outs when moving the mouse inside the button.
> > 
> > 3) Since the events are retargeted to the outside of the shadow boundary, the effective target of the mouseup and mousedown events is always going to be the button element.
> > 
> > 4) If so, the 5.2.3.2. section that Niwa-san mentioned makes perfect sense, relative to the outside of the shadow DOM boundary.
> 
> However, a button doesn't have a shadow DOM. So I have a problem with the premise in point 2.

Right, it currently doesn't. But it seems to fit pretty well, don't you think?

[Ryosuke Niwa]

(In reply to comment #14)
> (In reply to comment #13)
> > However, a button doesn't have a shadow DOM. So I have a problem with the premise in point 2.
> 
> Right, it currently doesn't. But it seems to fit pretty well, don't you think?

It really does. Only if we had a type of shadow host that acted like a normal node to JavaScript but acted like other shadow host inside WebCore...

[Dimitri Glazkov (Google)]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > However, a button doesn't have a shadow DOM. So I have a problem with the premise in point 2.
> > 
> > Right, it currently doesn't. But it seems to fit pretty well, don't you think?
> 
> It really does. Only if we had a type of shadow host that acted like a normal node to JavaScript but acted like other shadow host inside WebCore...

That's what <output> does. A button element is essentially this:

<template><output></output></template>

[Ojan Vafai]

I think for the purposes of getting the correct user-experience and for web-compat it is sufficient to fire the click event on the deepest common ancestor. We should start there. If we need to match other browsers behavior by special-casing button elements, we can always do so later.

[Ryosuke Niwa]

*** Bug 49297 has been marked as a duplicate of this bug. ***

[Brock Williams]

This same bug also happens on Links.  We have a QtWebKit based touchscreen app, and we have added a large padding to all link tags to give a larger target for tapping.  If the user starts the tap on the link text and finishes over the padding, the click event does not fire.  I can confirm that the patch attached fixes the issue for us on both links and button elements.

[Dominic Cooney]

I am unblocking this from bug 56211. From Comment 12, it sounds like implementing button to use Shadow DOM (ie bug 56211) would fix this, which indicates bug 56211 blocks this bug.

However given Comment 19 it seems this applies to elements where there's no Shadow DOM in question (ie spans).

*** Bug 113504 has been marked as a duplicate of this bug. ***

[Alexander Shalamov]

*** Bug 114417 has been marked as a duplicate of this bug. ***

The priority on this needs to be increased, it is beyond the pale that a fundamental component of the browser is so horribly broken three years after being reported, and with a clear reproducible test case available. The example attachment https://bugs.webkit.org/attachment.cgi?id=88775 perfectly illustrates the problem. 

The *buttons* are not registering *clicks* in a very common occurrence: the mouse moving a little when clicking. The sole purpose, the reason buttons exist, is for clicking. 

What the fuck, honestly?

[Jack O'Connor]

This is a very serious issue if the button gets transformed down 1px on click. In this case the bug no longer relies on the user accidentally dragging during the click - it happens when the user does a normal click at the very top or bottom of the text. I have explained the problem in this codepen: http://codepen.io/jackocnr/pen/yLFhD

[Kent Tamura]

FYI. We recently fix this problem in Blink. http://src.chromium.org/viewvc/blink?view=revision&revision=162081

[youenn fablet]

Created attachment 277685 [details]
Patch

[Build Bot]

Comment on attachment 277685 [details]
Patch

Attachment 277685 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1238936

New failing tests:
fast/events/click-over-descendant-elements.html

[Build Bot]

Created attachment 277687 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 277685 [details]
Patch

Attachment 277685 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1238947

New failing tests:
fast/events/click-over-descendant-elements.html

[Build Bot]

Created attachment 277689 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 277685 [details]
Patch

Attachment 277685 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1238999

New failing tests:
fast/events/click-over-descendant-elements.html

[Build Bot]

Created attachment 277691 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[youenn fablet]

Created attachment 277698 [details]
Patch

[youenn fablet]

(In reply to comment #25)
> FYI. We recently fix this problem in Blink.
> http://src.chromium.org/viewvc/blink?view=revision&revision=162081

Proposed patch is based on http://src.chromium.org/viewvc/blink?view=revision&revision=162081
It fixes the case of computing the ancestor.
It does not fix the case of deleted node between mouseDown/mouseUp event as can be seen from the expected.txt file.

[Ryosuke Niwa]

Comment on attachment 277698 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277698&action=review

> Source/WebCore/dom/Node.cpp:955
> +Node* Node::commonAncestorOverShadowBoundary(const Node* other)

I'd call this commonAncestorCrossingShadowBoundary instead to be consistent

> Source/WebCore/dom/Node.cpp:978
> +    for (Node* node = this; node; node = node->parentOrShadowHostNode()) {
> +        if (node == other)
> +            return node;
> +        thisDepth++;
> +    }
> +    int otherDepth = 0;
> +    for (const Node* node = other; node; node = node->parentOrShadowHostNode()) {
> +        if (node == this)
> +            return this;
> +        otherDepth++;
> +    }

This code is horribly inefficient when two nodes belong to two different tree scopes (e.g. one inside a shadow tree and another in a document tree).
We should start our search from the nodes in the lowest common tree scope and just use parentNode() in these loops.
You can probably either extend commonTreeScope (in TreeScope.cpp) or add a similar helper function for that.

[youenn fablet]

Created attachment 277988 [details]
Fixed according review

[youenn fablet]

(In reply to comment #36)
> Created attachment 277988 [details]
> Fixed according review

Thanks for the feedback.
I tried addressing it in the latest patch.

[Darin Adler]

Comment on attachment 277988 [details]
Fixed according review

View in context: https://bugs.webkit.org/attachment.cgi?id=277988&action=review

Some ideas for future refinement.

> Source/WebCore/dom/Node.cpp:968
> +    int thisDepth = 0;
> +    for (auto node = this; node; node = node->parentNode()) {
> +        if (node == &other)
> +            return node;
> +        thisDepth++;
> +    }
> +    int otherDepth = 0;
> +    for (auto node = &other; node; node = node->parentNode()) {
> +        if (node == this)
> +            return this;
> +        otherDepth++;
> +    }

Seeing code repeated twice like this makes it seem like we should have a helper function for this operation.

I would have used unsigned rather than int for this.

> Source/WebCore/dom/Node.cpp:970
> +    Node* thisIterator = this;
> +    const Node* otherIterator = &other;

I think the word for these local variables should be "ancestor" rather than "iterator".

> Source/WebCore/dom/Node.cpp:977
> +    if (thisDepth > otherDepth) {
> +        for (int i = thisDepth; i > otherDepth; --i)
> +            thisIterator = thisIterator->parentNode();
> +    } else if (otherDepth > thisDepth) {
> +        for (int i = otherDepth; i > thisDepth; --i)
> +            otherIterator = otherIterator->parentNode();
> +    }

I’d like to see a helper function that takes an unsigned and walks up in a loop. Would make this more readable:

    if (thisDepth > otherDepth)
        thisAncestor = ancestor(thisAncestor, otherDepth - thisDepth);
    else
        otherAncestor = ancestor(otherAncestor, thisDepth - otherDepth);

> Source/WebCore/dom/Node.cpp:983
> +    while (thisIterator) {
> +        if (thisIterator == otherIterator)
> +            return thisIterator;
> +        thisIterator = thisIterator->parentNode();
> +        otherIterator = otherIterator->parentNode();
> +    }

I think this might be better as a for loop.

> Source/WebCore/dom/Node.cpp:1002
> +    ASSERT(parentThis);

Why can we assert this? What guarantees it is true?

> Source/WebCore/dom/Node.cpp:1004
> +    ASSERT(parentOther);

Why can we assert this? What guarantees it is true?

> Source/WebCore/dom/Node.h:413
> +    Node* commonAncestor(const Node&);
> +    Node* commonAncestorCrossingShadowBoundary(Node*);

I agree that these are useful functions. But functions that give equal weight to two different objects aren’t great as member functions. Maybe consider non-member functions for these operations?

Why does the second function take a pointer rather than a reference? Why does the first function take a const& but the second take a non-const*?

[Ryosuke Niwa]

Comment on attachment 277988 [details]
Fixed according review

View in context: https://bugs.webkit.org/attachment.cgi?id=277988&action=review

> Source/WebCore/dom/Node.cpp:995
> +    Element* shadowHost = this->shadowHost();
> +    if (shadowHost && shadowHost == other->shadowHost())
> +        return shadowHost;

Could you add a FIXME here saying that this is wrong for author created shadow trees?

>> Source/WebCore/dom/Node.cpp:1002
>> +    ASSERT(parentThis);
> 
> Why can we assert this? What guarantees it is true?

Because scope is the common ancestor tree scope of "this" and "other".

[youenn fablet]

Created attachment 278077 [details]
Patch for landing

[youenn fablet]

Thanks for the review.

> > Source/WebCore/dom/Node.cpp:968
> > +    int thisDepth = 0;
> > +    for (auto node = this; node; node = node->parentNode()) {
> > +        if (node == &other)
> > +            return node;
> > +        thisDepth++;
> > +    }
> > +    int otherDepth = 0;
> > +    for (auto node = &other; node; node = node->parentNode()) {
> > +        if (node == this)
> > +            return this;
> > +        otherDepth++;
> > +    }
> 
> Seeing code repeated twice like this makes it seem like we should have a
> helper function for this operation.

The issue is that other is const and this is not const.
I removed constness of other (see below) but it might make sense to add it again after some refactoring.

> I would have used unsigned rather than int for this.

OK

> > Source/WebCore/dom/Node.cpp:970
> > +    Node* thisIterator = this;
> > +    const Node* otherIterator = &other;
> 
> I think the word for these local variables should be "ancestor" rather than
> "iterator".

OK

> > Source/WebCore/dom/Node.cpp:977
> > +    if (thisDepth > otherDepth) {
> > +        for (int i = thisDepth; i > otherDepth; --i)
> > +            thisIterator = thisIterator->parentNode();
> > +    } else if (otherDepth > thisDepth) {
> > +        for (int i = otherDepth; i > thisDepth; --i)
> > +            otherIterator = otherIterator->parentNode();
> > +    }
> 
> I’d like to see a helper function that takes an unsigned and walks up in a
> loop. Would make this more readable:
> 
>     if (thisDepth > otherDepth)
>         thisAncestor = ancestor(thisAncestor, otherDepth - thisDepth);
>     else
>         otherAncestor = ancestor(otherAncestor, thisDepth - otherDepth);

OK

> > Source/WebCore/dom/Node.cpp:983
> > +    while (thisIterator) {
> > +        if (thisIterator == otherIterator)
> > +            return thisIterator;
> > +        thisIterator = thisIterator->parentNode();
> > +        otherIterator = otherIterator->parentNode();
> > +    }
> 
> I think this might be better as a for loop.

OK

> > Source/WebCore/dom/Node.h:413
> > +    Node* commonAncestor(const Node&);
> > +    Node* commonAncestorCrossingShadowBoundary(Node*);
> 
> I agree that these are useful functions. But functions that give equal
> weight to two different objects aren’t great as member functions. Maybe
> consider non-member functions for these operations?
> 
> Why does the second function take a pointer rather than a reference? Why
> does the first function take a const& but the second take a non-const*?

commonTreeScope is expecting a non-const unfortunately.

For consistency, I changed to:
Node* commonAncestor(Node&, Node&);
Node* commonAncestorCrossingShadowBoundary(Node&, Node&);
If we were improving commonTreeScope to accept the last parameter as const, the second parameter of these methods could be made const as well.

> > Source/WebCore/dom/Node.cpp:995
> > +    Element* shadowHost = this->shadowHost();
> > +    if (shadowHost && shadowHost == other->shadowHost())
> > +        return shadowHost;
> 
> Could you add a FIXME here saying that this is wrong for author created
> shadow trees?

OK

[WebKit Commit Bot]

Comment on attachment 278077 [details]
Patch for landing

Clearing flags on attachment: 278077

Committed r200414: <http://trac.webkit.org/changeset/200414>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Reverted r200414 for reason:

This change appears to have broken the 'write a reply' field on Nextdoor.com

Committed r201292: <http://trac.webkit.org/changeset/201292>

[Ryan Haddad]

Created attachment 279582 [details]
Reduction case for Nextdoor regression

1. Mouse down inside the red square.
2. Move the mouse outside of the square, onto the white page background.
3. Mouse up.

Expected results: Nothing.
Actual results: The page tells you that you clicked outside the square.

[youenn fablet]

(In reply to comment #45)
> Created attachment 279582 [details]
> Reduction case for Nextdoor regression
> 
> 1. Mouse down inside the red square.
> 2. Move the mouse outside of the square, onto the white page background.
> 3. Mouse up.
> 
> Expected results: Nothing.
> Actual results: The page tells you that you clicked outside the square.

Is this case similar to the FAIL subtest in LayoutTests/fast/events/click-over-descendant-elements-expected.txt?
A colleague of mine started investigating this FAIL test case and will try to propose a patch next week. Plan is to handle it in a separate bug.

[Jon Lee]

Youenn, is there a bug # for that?

Now that this patch has been rolled out, please determine whether that subtest is indeed tied to this bug, and post a new patch that addresses, at least, the test case that Ryan provided.

rdar://problem/26270328

[youenn fablet]

(In reply to comment #47)
> Youenn, is there a bug # for that?

Not yet.

> Now that this patch has been rolled out, please determine whether that
> subtest is indeed tied to this bug, and post a new patch that addresses, at
> least, the test case that Ryan provided.
> 
> rdar://problem/26270328

Checking the test now, it is not related to the expected missing feature.
AFAIUI, the rolled-out patch conforms to the standard.
But I guess this is causing a compatibility issue.
 
Testing the reduction case Ryan posted, firefox is passing the test but chrome and IE are failing it.

I'll investigate it further.
Ryan, would you be able to share the not-reduced nextdoor failure?

[youenn fablet]

According https://codereview.chromium.org/110173005, one cause might be related to mousing down on a form control and then mousing up outside of it.

[Ryan Haddad]

Created attachment 279659 [details]
Webarchive of Nextdoor

Within the attached webarchive, click on one of the "Write a reply" text fields to reproduce the issue. The text field enlarges when the click is held, but collapses again when the click is released.

This issue does not reproduce before r200414.

[youenn fablet]

Created attachment 279773 [details]
Adding supportsFocus check

[youenn fablet]

(In reply to comment #50)
> Created attachment 279659 [details]
> Webarchive of Nextdoor
> 
> Within the attached webarchive, click on one of the "Write a reply" text
> fields to reproduce the issue. The text field enlarges when the click is
> held, but collapses again when the click is released.
> 
> This issue does not reproduce before r200414.

I uploaded a patch that I hope is solving the issue (but not the reduction case).
Ryan, I was not able to test the web archive.
Would you be able to test the patch with it?

[Ricky Mondello]

(In reply to comment #52)
> (In reply to comment #50)
> > Created attachment 279659 [details]
> > Webarchive of Nextdoor
> > 
> > Within the attached webarchive, click on one of the "Write a reply" text
> > fields to reproduce the issue. The text field enlarges when the click is
> > held, but collapses again when the click is released.
> > 
> > This issue does not reproduce before r200414.
> 
> I uploaded a patch that I hope is solving the issue (but not the reduction
> case).
> Ryan, I was not able to test the web archive.
> Would you be able to test the patch with it?

Does it not solve the reduction case (the red square) because the reduction is incorrect or invalid somehow, or because the fix needs improvement?

[youenn fablet]

(In reply to comment #53)
> (In reply to comment #52)
> > (In reply to comment #50)
> > > Created attachment 279659 [details]
> > > Webarchive of Nextdoor
> > > 
> > > Within the attached webarchive, click on one of the "Write a reply" text
> > > fields to reproduce the issue. The text field enlarges when the click is
> > > held, but collapses again when the click is released.
> > > 
> > > This issue does not reproduce before r200414.
> > 
> > I uploaded a patch that I hope is solving the issue (but not the reduction
> > case).
> > Ryan, I was not able to test the web archive.
> > Would you be able to test the patch with it?
> 
> Does it not solve the reduction case (the red square) because the reduction
> is incorrect or invalid somehow, or because the fix needs improvement?

The reduction test does not pass in Chrome and IE. It passes on Firefox. According my understanding of the spec, it should not pass.

[Jon Lee]

Created attachment 279799 [details]
Nextdoor webarchive extracted to simple zip

[youenn fablet]

(In reply to comment #55)
> Created attachment 279799 [details]
> Nextdoor webarchive extracted to simple zip

Thanks for the zip.
The proposed patch fixes the nextdoor bug.
The proposed patch also fixes the reduction case in the case the red square is changed to a button. In that case, both Chrome and Firefox agrees to not dispatch any event.

[youenn fablet]

Comment on attachment 279773 [details]
Adding supportsFocus check

View in context: https://bugs.webkit.org/attachment.cgi?id=279773&action=review

> Source/WebCore/dom/Node.cpp:982
> +        if (node->isElementNode() && downcast<Element>(node)->supportsFocus())

The criteria to return true is not very clear to me.
I guess supportsFocus or isFocusable might make sense.
Any thought?

> Source/WebCore/dom/Node.cpp:1012
> +        return nullptr;

These 5 lines are introduced to cover the nextdoor case.
This is the only major change compared to the previous patch.

Maybe commonAncestorCrossingShadowBoundary (without the new parameter) should go in EventHandler since it is becoming quite specific to event handling.

[Ryosuke Niwa]

For Apple reviewers, this patch also caused <rdar://problem/26185375>.

[Brady Eidson]

Comment on attachment 279773 [details]
Adding supportsFocus check

r-, as this has been pending review for over a year now. It is near-impossible that this patch still applies to trunk and unlikely to still be relevant in its current form.