Bug 52065

Summary: REGRESSION(r71934): Shadow DOM nodes leak via relatedTarget
Product: WebKit Reporter: Jonathan Vingiano <jvingiano>
Component: JavaScriptCoreAssignee: Dimitri Glazkov (Google) <dglazkov>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, commit-queue, dglazkov, eric, priyajeet.hora, temp01irc+bugzilla, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://jsfiddle.net/qs9f2/
Bug Depends on:    
Bug Blocks: 54025, 46015    
Attachments:
Description Flags
Patch darin: review+, commit-queue: commit-queue-

Jonathan Vingiano
Reported 2011-01-07 09:23:15 PST
Other browsers tested: Safari 5.0.3: OK Firefox 3.6.13: OK Opera 10.63: OK Opera 11: OK Rockmelt 0.8.40.147: OK What steps will reproduce the problem? 1. Create a div with an <input> as a child element. 2. Attach a mouseenter/leave event to this div. 3. Chrome will see a mouse over of the child input as a mouseleave event, even though it is a child of the parent <div> What is the expected result? Expected that Chrome will not consider mousing over the child <input> field as a mouseleave event of the parent div. What happens instead? Chrome considers the mousing over the child <input> as a mouseleave event even though it is not. Please provide any additional information below. Attach a screenshot if possible. http://jsfiddle.net/qs9f2/ http://bugs.jquery.com/ticket/7729
Attachments
Patch (4.69 KB, patch)
2011-02-08 13:02 PST, Dimitri Glazkov (Google) 		darin: review+
commit-queue: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Darth
Comment 1 2011-01-08 00:18:01 PST
This bug applies to all platforms btw.
Jonathan Vingiano
Comment 2 2011-01-08 07:53:10 PST
(In reply to comment #1) > This bug applies to all platforms btw. Figured but I couldn't test on any other platform so I didn't want to speak too soon. Will change it, thanks for testing.
Darth
Comment 3 2011-01-14 13:40:19 PST
Another site that seems to be exhibiting this issue http://www.movietickets.com/default.asp Mouse over the choose your movie box in the center top. This will open up a overlay div that has an input box, mouse over the input box and you will see the overlay disappears.
Jonathan Vingiano
Comment 4 2011-02-07 13:04:57 PST
Another realworld example of this bug has been pointed out at http://www.desiringgod.org After hovering over the sign-in link, a field will appear with input fields. Hovering over an input field will hide the whole div, which is unexpected UX and does not happen in non-Webkit browsers.
Dimitri Glazkov (Google)
Comment 5 2011-02-08 09:04:41 PST
Whoopsie.
Dimitri Glazkov (Google)
Comment 6 2011-02-08 09:41:07 PST
Here's the reduction: http://jsfiddle.net/dglazkov/qs9f2/20/
Dimitri Glazkov (Google)
Comment 7 2011-02-08 13:02:46 PST
Created attachment 81679 [details] Patch
Darin Adler
Comment 8 2011-02-08 13:22:51 PST
Comment on attachment 81679 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81679&action=review > Source/WebCore/dom/Node.cpp:2876 > - RefPtr<Node> relatedTarget = relatedTargetArg; > + RefPtr<Node> relatedTarget = pullOutOfShadow(relatedTargetArg); Is there really just one level or this? Doesn’t the event travel out of multiple layers of shadow, each time needing a different relatedTarget?
Dimitri Glazkov (Google)
Comment 9 2011-02-08 14:13:06 PST
(In reply to comment #8) > (From update of attachment 81679 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81679&action=review > > > Source/WebCore/dom/Node.cpp:2876 > > - RefPtr<Node> relatedTarget = relatedTargetArg; > > + RefPtr<Node> relatedTarget = pullOutOfShadow(relatedTargetArg); > > Is there really just one level or this? Doesn’t the event travel out of multiple layers of shadow, each time needing a different relatedTarget? The relatedTarget only needs to move if we have (In reply to comment #8) > (From update of attachment 81679 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81679&action=review > > > Source/WebCore/dom/Node.cpp:2876 > > - RefPtr<Node> relatedTarget = relatedTargetArg; > > + RefPtr<Node> relatedTarget = pullOutOfShadow(relatedTargetArg); > > Is there really just one level or this? Doesn’t the event travel out of multiple layers of shadow, each time needing a different relatedTarget? This is not the best or final solution, but it's enough to stop the leaks and won't exhibit gross side effects in the current uses of shadow DOM. The event will either be stopped if the relatedTarget is inside of the same shadow boundary as target or retargeted to the boundary that's closest to the target. Doing this without DOM scopes (see bug 52963) is possible but super-tedious.
Dimitri Glazkov (Google)
Comment 10 2011-02-08 14:15:29 PST
(In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 81679 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=81679&action=review > > > > > Source/WebCore/dom/Node.cpp:2876 > > > - RefPtr<Node> relatedTarget = relatedTargetArg; > > > + RefPtr<Node> relatedTarget = pullOutOfShadow(relatedTargetArg); > > > > Is there really just one level or this? Doesn’t the event travel out of multiple layers of shadow, each time needing a different relatedTarget? > > The relatedTarget only needs to move if we have (In reply to comment #8) > > (From update of attachment 81679 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=81679&action=review > > > > > Source/WebCore/dom/Node.cpp:2876 > > > - RefPtr<Node> relatedTarget = relatedTargetArg; > > > + RefPtr<Node> relatedTarget = pullOutOfShadow(relatedTargetArg); > > > > Is there really just one level or this? Doesn’t the event travel out of multiple layers of shadow, each time needing a different relatedTarget? > > This is not the best or final solution, but it's enough to stop the leaks and won't exhibit gross side effects in the current uses of shadow DOM. > > The event will either be stopped if the relatedTarget is inside of the same shadow boundary as target or retargeted to the boundary that's closest to the target. Doing this without DOM scopes (see bug 52963) is possible but super-tedious. Oh wow, I clearly over-edited this :) The last paragraph describes the proper solution.
WebKit Commit Bot
Comment 11 2011-02-08 21:56:03 PST
Comment on attachment 81679 [details] Patch Rejecting attachment 81679 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2 Last 500 characters of output: ng/hebrew ....... fast/events ........................................................................................................................................................................................................................................ fast/events/shadow-boundary-crossing.html -> failed Exiting early after 1 failures. 8052 tests run. 170.56s total testing time 8051 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 4 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/7791271
Dimitri Glazkov (Google)
Comment 12 2011-02-08 22:05:08 PST
Ah. I should really update results foist. :)
Dimitri Glazkov (Google)
Comment 13 2011-02-09 10:03:46 PST
Committed r78077: <http://trac.webkit.org/changeset/78077>
WebKit Review Bot
Comment 14 2011-02-09 13:23:33 PST
http://trac.webkit.org/changeset/78077 might have broken GTK Linux 64-bit Debug
Dimitri Glazkov (Google)
Comment 15 2011-02-23 15:50:19 PST
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > (From update of attachment 81679 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=81679&action=review > > > > > > > Source/WebCore/dom/Node.cpp:2876 > > > > - RefPtr<Node> relatedTarget = relatedTargetArg; > > > > + RefPtr<Node> relatedTarget = pullOutOfShadow(relatedTargetArg); > > > > > > Is there really just one level or this? Doesn’t the event travel out of multiple layers of shadow, each time needing a different relatedTarget? > > > > The relatedTarget only needs to move if we have (In reply to comment #8) > > > (From update of attachment 81679 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=81679&action=review > > > > > > > Source/WebCore/dom/Node.cpp:2876 > > > > - RefPtr<Node> relatedTarget = relatedTargetArg; > > > > + RefPtr<Node> relatedTarget = pullOutOfShadow(relatedTargetArg); > > > > > > Is there really just one level or this? Doesn’t the event travel out of multiple layers of shadow, each time needing a different relatedTarget? > > > > This is not the best or final solution, but it's enough to stop the leaks and won't exhibit gross side effects in the current uses of shadow DOM. > > > > The event will either be stopped if the relatedTarget is inside of the same shadow boundary as target or retargeted to the boundary that's closest to the target. Doing this without DOM scopes (see bug 52963) is possible but super-tedious. > > Oh wow, I clearly over-edited this :) > > The last paragraph describes the proper solution. ... and it turns out I need it sooner than I thought.
Note You need to log in before you can comment on or make changes to this bug.