Bug 60600

Summary: click event shouldn't fire when the target is ever removed in mouseup
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: UI EventsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, arv, commit-queue, darin, eric, glenn, mjs, ojan, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 60833    
Bug Blocks:    
Attachments:
Description Flags
fixes the bug
none
Added instructions for manual testing
none
benchmark none

Ryosuke Niwa
Reported 2011-05-10 18:12:41 PDT
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.
Attachments
fixes the bug (11.65 KB, patch)
2011-05-10 19:34 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Added instructions for manual testing (11.75 KB, patch)
2011-05-10 19:59 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
benchmark (1.16 KB, text/html)
2011-05-11 04:42 PDT, Ryosuke Niwa 		no flags
Details
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2011-05-10 18:13:42 PDT
See discussions on http://lists.w3.org/Archives/Public/www-dom/2011AprJun/0058.html
Ryosuke Niwa
Comment 2 2011-05-10 19:34:32 PDT
Created attachment 93063 [details] fixes the bug
Erik Arvidsson
Comment 3 2011-05-10 19:46:24 PDT
Comment on attachment 93063 [details] fixes the bug 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?
Ryosuke Niwa
Comment 4 2011-05-10 19:50:14 PDT
(In reply to comment #3) > (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? Yes but I don't think this is a big deal. There are many O(n) operations in call sites of this function already.
Ryosuke Niwa
Comment 5 2011-05-10 19:58:09 PDT
(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...
Ryosuke Niwa
Comment 6 2011-05-10 19:59:40 PDT
Created attachment 93064 [details] Added instructions for manual testing
Ryosuke Niwa
Comment 7 2011-05-11 04:42:47 PDT
Created attachment 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.
Ryosuke Niwa
Comment 8 2011-05-13 12:02:49 PDT
Can anyone review this patch?
Ryosuke Niwa
Comment 9 2011-05-13 13:39:13 PDT
Yay! Thanks for the review, Darin!
WebKit Commit Bot
Comment 10 2011-05-13 14:35:19 PDT
Comment on attachment 93064 [details] Added instructions for manual testing Clearing flags on attachment: 93064 Committed r86461: <http://trac.webkit.org/changeset/86461>
WebKit Commit Bot
Comment 11 2011-05-13 14:35:25 PDT
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 12 2011-05-13 17:33:44 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.