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.
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)
Confirmed on Nightly WebKit. The bug does not reproduce on Firefox 4.
IE8 behaves as same as Firefox. Ok, we should fix this.
"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.
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.
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.
I'm going to assign the bug to myself at least until I get some response on www-dom.
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.
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.
(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.
(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?
(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...
(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>
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.
*** Bug 49297 has been marked as a duplicate of this bug. ***
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.
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. ***
*** 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?
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
FYI. We recently fix this problem in Blink. http://src.chromium.org/viewvc/blink?view=revision&revision=162081
Created attachment 277685 [details] Patch
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
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 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
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 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
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
Created attachment 277698 [details] Patch
(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 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.
Created attachment 277988 [details] Fixed according review
(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 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 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".
Created attachment 278077 [details] Patch for landing
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 on attachment 278077 [details] Patch for landing Clearing flags on attachment: 278077 Committed r200414: <http://trac.webkit.org/changeset/200414>
All reviewed patches have been landed. Closing bug.
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>
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.
(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.
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
(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?
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.
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.
Created attachment 279773 [details] Adding supportsFocus check
(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?
(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?
(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.
Created attachment 279799 [details] Nextdoor webarchive extracted to simple zip
(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 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.
For Apple reviewers, this patch also caused <rdar://problem/26185375>.
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.