Bug 60600 - click event shouldn't fire when the target is ever removed in mouseup
: click event shouldn't fire when the target is ever removed in mouseup
Status: RESOLVED FIXED
: WebKit
Event Handling
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 60833
:
  Show dependency treegraph
 
Reported: 2011-05-10 18:12 PST by
Modified: 2011-05-14 01:38 PST (History)


Attachments
fixes the bug (11.65 KB, patch)
2011-05-10 19:34 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
Added instructions for manual testing (11.75 KB, patch)
2011-05-10 19:59 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
benchmark (1.16 KB, text/html)
2011-05-11 04:42 PST, Ryosuke Niwa
no flags Details


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-05-10 18:12:41 PST
WebKit currently fires click event even on a node that was removed or once removed from its document during mouseup event.  However, both Firefox and Internet Explorer do NOT fire click event in such cases.  We should match WebKit's behavior to those two browsers to be consistent.
------- Comment #1 From 2011-05-10 18:13:42 PST -------
See discussions on http://lists.w3.org/Archives/Public/www-dom/2011AprJun/0058.html
------- Comment #2 From 2011-05-10 19:34:32 PST -------
Created an attachment (id=93063) [details]
fixes the bug
------- Comment #3 From 2011-05-10 19:46:24 PST -------
(From update of attachment 93063 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=93063&action=review

> Source/WebCore/page/EventHandler.cpp:263
> +    if (nodeToBeRemoved->contains(m_clickNode.get()))

contains is O(depth). Will this cause all removeChild to have to walk up the dom tree?
------- Comment #4 From 2011-05-10 19:50:14 PST -------
(In reply to comment #3)
> (From update of attachment 93063 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93063&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:263
> > +    if (nodeToBeRemoved->contains(m_clickNode.get()))
> 
> contains is O(depth). Will this cause all removeChild to have to walk up the dom tree?

Yes but I don't think this is a big deal. There are many O(n) operations in call sites of this function already.
------- Comment #5 From 2011-05-10 19:58:09 PST -------
(In reply to comment #4)
> (In reply to comment #3)
> > contains is O(depth). Will this cause all removeChild to have to walk up the dom tree?
> 
> Yes but I don't think this is a big deal. There are many O(n) operations in call sites of this function already.

An alternative is to wait until the node is actually removed and check inDocument() but that's tricky to get right because there doesn't seem to be any precedents set by other classes and the node can be inserted back or moved to a different node during mutation events, etc...
------- Comment #6 From 2011-05-10 19:59:40 PST -------
Created an attachment (id=93064) [details]
Added instructions for manual testing
------- Comment #7 From 2011-05-11 04:42:47 PST -------
Created an attachment (id=93104) [details]
benchmark

I made a benchmark for this patch.  So there seems to be some performance impact in very specific case where selection isn't where m_clickNode is and m_clickNode is really deep. On my MacBookPro, WebKit ToT Mac port runs the benchmark in 2ms without the patch but takes 7ms with the patch.  However, when selection isn't moved manually by removeAllRanges(), the selection's nodeWillBeRemoved will dwarfs the effect of this patch making the benchmark to take ~170ms.  In practice, however, I don't think it's common for websites to have such a deep DOM structure, and put selection away from where user clicked and remove thousands of nodes between mousedown & mouseup events.
------- Comment #8 From 2011-05-13 12:02:49 PST -------
Can anyone review this patch?
------- Comment #9 From 2011-05-13 13:39:13 PST -------
Yay! Thanks for the review, Darin!
------- Comment #10 From 2011-05-13 14:35:19 PST -------
(From update of attachment 93064 [details])
Clearing flags on attachment: 93064

Committed r86461: <http://trac.webkit.org/changeset/86461>
------- Comment #11 From 2011-05-13 14:35:25 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #12 From 2011-05-13 17:33:44 PST -------
New behavior is spec'ed in http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-mouseevent-event-order

See "Example" part at the end of the section.