Bug 39620 - Clicks inside button elements are sometimes discarded when the mouse moves
: Clicks inside button elements are sometimes discarded when the mouse moves
Status: REOPENED
Product: WebKit
Classification: Unclassified
Component: Forms
: 528+ (Nightly build)
: Macintosh Intel OS X 10.6
: P2 Normal
Assigned To: youenn fablet
: NeedsReduction
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-24 14:42 PDT by bryan
Modified: 2016-05-26 08:04 PDT (History)
29 users (show)

See Also:


Attachments
image of a button with inner text box (3.95 KB, image/png)
2010-05-24 14:42 PDT, bryan
no flags Details
Simple Page which demostrates this bug. (831 bytes, text/html)
2011-04-08 00:02 PDT, ted1
no flags Details
work in progress (4.02 KB, patch)
2011-05-11 07:41 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (12.02 KB, patch)
2016-04-29 02:31 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (1.02 MB, application/zip)
2016-04-29 03:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (657.13 KB, application/zip)
2016-04-29 03:34 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (870.91 KB, application/zip)
2016-04-29 03:53 PDT, Build Bot
no flags Details
Patch (13.09 KB, patch)
2016-04-29 06:49 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Fixed according review (13.36 KB, patch)
2016-05-03 02:15 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (13.50 KB, patch)
2016-05-04 04:49 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Reduction case for Nextdoor regression (367 bytes, text/html)
2016-05-23 13:29 PDT, Ryan Haddad
no flags Details
Webarchive of Nextdoor (1.27 MB, application/zip)
2016-05-24 09:32 PDT, Ryan Haddad
no flags Details
Adding supportsFocus check (15.43 KB, patch)
2016-05-25 08:21 PDT, youenn fablet
youennf: review?
Details | Formatted Diff | Diff
Nextdoor webarchive extracted to simple zip (1.37 MB, application/zip)
2016-05-25 13:29 PDT, Jon Lee
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description bryan 2010-05-24 14:42:11 PDT
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.
Comment 1 Alexey Proskuryakov 2010-05-24 22:07:25 PDT
Someone needs to grab this bit of HTML/CSS code to investigate.
Comment 2 ted1 2011-04-08 00:02:54 PDT
Created attachment 88775 [details]
Simple Page which demostrates this bug.

This pages shows the bug. It works correctly in Firefox 3.6.
Comment 3 ted1 2011-04-08 00:05:45 PDT
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)
Comment 4 Ryosuke Niwa 2011-04-11 13:33:08 PDT
Confirmed on Nightly WebKit. The bug does not reproduce on Firefox 4.
Comment 5 Kent Tamura 2011-04-11 19:59:32 PDT
IE8 behaves as same as Firefox.
Ok, we should fix this.
Comment 6 Ryosuke Niwa 2011-04-14 17:10:13 PDT
"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.
Comment 7 Alexey Proskuryakov 2011-04-14 17:20:23 PDT
User experience is definitely wrong here, so this is something that needs to be fixed one way or another.
Comment 8 bryan 2011-04-14 17:21:37 PDT
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.
Comment 9 Ryosuke Niwa 2011-04-14 17:50:19 PDT
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.
Comment 10 Ryosuke Niwa 2011-04-14 17:53:06 PDT
I'm going to assign the bug to myself at least until I get some response on www-dom.
Comment 11 Ryosuke Niwa 2011-05-11 07:41:34 PDT
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.
Comment 12 Dimitri Glazkov (Google) 2011-05-11 09:59:40 PDT
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.
Comment 13 Ryosuke Niwa 2011-05-11 10:40:52 PDT
(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.
Comment 14 Dimitri Glazkov (Google) 2011-05-11 10:50:35 PDT
(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?
Comment 15 Ryosuke Niwa 2011-05-11 10:55:36 PDT
(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...
Comment 16 Dimitri Glazkov (Google) 2011-05-11 11:00:31 PDT
(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>
Comment 17 Ojan Vafai 2011-06-02 16:34:56 PDT
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.
Comment 18 Ryosuke Niwa 2011-08-15 14:19:04 PDT
*** Bug 49297 has been marked as a duplicate of this bug. ***
Comment 19 Brock Williams 2012-05-09 09:11:21 PDT
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.
Comment 20 Dominic Cooney 2012-12-16 18:32:22 PST
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).
Comment 21 ernests 2013-03-28 23:34:16 PDT
*** Bug 113504 has been marked as a duplicate of this bug. ***
Comment 22 Alexander Shalamov 2013-04-11 23:17:34 PDT
*** Bug 114417 has been marked as a duplicate of this bug. ***
Comment 23 hugh.lomas 2013-11-12 10:32:54 PST
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?
Comment 24 Jack O'Connor 2013-11-28 12:20:52 PST
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
Comment 25 Kent Tamura 2013-11-28 15:31:39 PST
FYI. We recently fix this problem in Blink. http://src.chromium.org/viewvc/blink?view=revision&revision=162081
Comment 26 youenn fablet 2016-04-29 02:31:24 PDT
Created attachment 277685 [details]
Patch
Comment 27 Build Bot 2016-04-29 03:25:09 PDT
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
Comment 28 Build Bot 2016-04-29 03:25:17 PDT
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
Comment 29 Build Bot 2016-04-29 03:34:12 PDT
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
Comment 30 Build Bot 2016-04-29 03:34:20 PDT
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
Comment 31 Build Bot 2016-04-29 03:53:06 PDT
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
Comment 32 Build Bot 2016-04-29 03:53:14 PDT
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
Comment 33 youenn fablet 2016-04-29 06:49:06 PDT
Created attachment 277698 [details]
Patch
Comment 34 youenn fablet 2016-04-29 08:04:04 PDT
(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.
Comment 35 Ryosuke Niwa 2016-04-29 20:44:34 PDT
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.
Comment 36 youenn fablet 2016-05-03 02:15:19 PDT
Created attachment 277988 [details]
Fixed according review
Comment 37 youenn fablet 2016-05-03 02:15:52 PDT
(In reply to comment #36)
> Created attachment 277988 [details]
> Fixed according review

Thanks for the feedback.
I tried addressing it in the latest patch.
Comment 38 Darin Adler 2016-05-03 08:22:14 PDT
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*?
Comment 39 Ryosuke Niwa 2016-05-03 11:08:07 PDT
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".
Comment 40 youenn fablet 2016-05-04 04:49:35 PDT
Created attachment 278077 [details]
Patch for landing
Comment 41 youenn fablet 2016-05-04 04:54:07 PDT
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
Comment 42 WebKit Commit Bot 2016-05-04 05:50:51 PDT
Comment on attachment 278077 [details]
Patch for landing

Clearing flags on attachment: 278077

Committed r200414: <http://trac.webkit.org/changeset/200414>
Comment 43 WebKit Commit Bot 2016-05-04 05:50:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 44 Ryan Haddad 2016-05-23 13:24:00 PDT
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>
Comment 45 Ryan Haddad 2016-05-23 13:29:13 PDT
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.
Comment 46 youenn fablet 2016-05-23 13:36:26 PDT
(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.
Comment 47 Jon Lee 2016-05-23 15:10:31 PDT
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
Comment 48 youenn fablet 2016-05-24 00:24:52 PDT
(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?
Comment 49 youenn fablet 2016-05-24 00:31:26 PDT
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.
Comment 50 Ryan Haddad 2016-05-24 09:32:07 PDT
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.
Comment 51 youenn fablet 2016-05-25 08:21:36 PDT
Created attachment 279773 [details]
Adding supportsFocus check
Comment 52 youenn fablet 2016-05-25 08:23:15 PDT
(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?
Comment 53 Ricky Mondello 2016-05-25 09:43:23 PDT
(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?
Comment 54 youenn fablet 2016-05-25 10:29:29 PDT
(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.
Comment 55 Jon Lee 2016-05-25 13:29:25 PDT
Created attachment 279799 [details]
Nextdoor webarchive extracted to simple zip
Comment 56 youenn fablet 2016-05-26 07:51:06 PDT
(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.
Comment 57 youenn fablet 2016-05-26 08:04:00 PDT
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.