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-

Description Jonathan Vingiano 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
Comment 1 Darth 2011-01-08 00:18:01 PST
This bug applies to all platforms btw.
Comment 2 Jonathan Vingiano 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.
Comment 3 Darth 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.
Comment 4 Jonathan Vingiano 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.
Comment 5 Dimitri Glazkov (Google) 2011-02-08 09:04:41 PST
Whoopsie.
Comment 6 Dimitri Glazkov (Google) 2011-02-08 09:41:07 PST
Here's the reduction: http://jsfiddle.net/dglazkov/qs9f2/20/
Comment 7 Dimitri Glazkov (Google) 2011-02-08 13:02:46 PST
Created attachment 81679 [details]
Patch
Comment 8 Darin Adler 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?
Comment 9 Dimitri Glazkov (Google) 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.
Comment 10 Dimitri Glazkov (Google) 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.
Comment 11 WebKit Commit Bot 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
Comment 12 Dimitri Glazkov (Google) 2011-02-08 22:05:08 PST
Ah. I should really update results foist. :)
Comment 13 Dimitri Glazkov (Google) 2011-02-09 10:03:46 PST
Committed r78077: <http://trac.webkit.org/changeset/78077>
Comment 14 WebKit Review Bot 2011-02-09 13:23:33 PST
http://trac.webkit.org/changeset/78077 might have broken GTK Linux 64-bit Debug
Comment 15 Dimitri Glazkov (Google) 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.