Bug 55515

Summary: Implement proper handling of mouseover/mouseout events in regard to shadow DOM boundaries.
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: New BugsAssignee: Dimitri Glazkov (Google) <dglazkov>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, eric, jamesr, ojan, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 57045, 57050, 57168, 57247, 57285, 57391, 57402, 57419, 57521, 57562, 57639, 57642, 57655, 58190    
Bug Blocks: 53020    
Attachments:
Description Flags
Patch
none
Using EventDispatcher.
none
Ready for review.
none
Addressed feedback.
none
Adjusted indent
none
More feedback addressed. ojan: review+, ojan: commit-queue-

Description Dimitri Glazkov (Google) 2011-03-01 15:09:08 PST
Implement proper handling of mouseover/mouseout events in regard to shadow DOM boundaries.
Comment 1 Dimitri Glazkov (Google) 2011-03-01 15:19:20 PST
Created attachment 84307 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2011-03-01 15:23:53 PST
I know this patch is _G_narly. I really don't like digging into the type of the event at this point. All of the abstractions we built so far attempt to insulate dispatch logic from the event type differences.

I'll keep thinking on better ideas.
Comment 3 Dimitri Glazkov (Google) 2011-03-23 16:37:30 PDT
One solution is to move retargeting into the event. Let me see how that works.
Comment 4 Dimitri Glazkov (Google) 2011-03-24 14:04:22 PDT
Comment on attachment 84307 [details]
Patch

removing r? flag for now. Refactoring event code..
Comment 5 Dimitri Glazkov (Google) 2011-04-02 08:34:45 PDT
Created attachment 87975 [details]
Using EventDispatcher.
Comment 6 Dimitri Glazkov (Google) 2011-04-05 11:03:16 PDT
Created attachment 88279 [details]
Ready for review.
Comment 7 Ryosuke Niwa 2011-04-07 09:49:25 PDT
Comment on attachment 88279 [details]
Ready for review.

View in context: https://bugs.webkit.org/attachment.cgi?id=88279&action=review

While I'm not familiar with XBL2.0 enough to r+ this patch, I'm going to leave some general feedback.

> Source/WebCore/dom/EventDispatcher.cpp:174
> +    // Calculate early if the common boundary is even possible by looking at
> +    // ancestors size and if the retargeting has occured (indicating the presence of shadow DOM boundaries).
> +    // If there are no boundaries detected, the target and related target can't have a common boundary.
> +    bool noCommonBoundary = m_ancestors.isEmpty() || m_ancestors.first().node() == m_ancestors.last().node();

I'd extract a helper inline function for this.

> Source/WebCore/dom/EventDispatcher.cpp:176
> +    // Build the ancestor chain for relatedTarget.

I think it's self-evident from the code that you're making a list of ancestors.

> Source/WebCore/dom/EventDispatcher.cpp:221
> +    Vector<EventContext>::iterator targetAncestor = m_ancestors.end();
> +    // Walk down from the top, looking for lowest common ancestor, also monitoring shadow DOM boundaries.
> +    bool diverged = false;
> +    for (Vector<Node*>::iterator i = relatedTargetAncestors.end() - 1; i >= relatedTargetAncestors.begin(); --i) {
> +        if (diverged) {
> +            if ((*i)->isShadowRoot()) {
> +                firstInnerBoundary = i + 1;
> +                break;
> +            }
> +            continue;
> +        }
> +
> +        if (targetAncestor == m_ancestors.begin()) {
> +            diverged = true;
> +            continue;
> +        }
> +
> +        targetAncestor--;
> +
> +        if ((*i)->isShadowRoot())
> +            lastCommonBoundary = targetAncestor;
> +
> +        if ((*i) != (*targetAncestor).node()) {
> +            targetAncestor = m_ancestors.begin();
> +            diverged = true;
> +            continue;
> +        }
> +    }

I'd extract this as a function.
Comment 8 Dimitri Glazkov (Google) 2011-04-08 10:55:35 PDT
Created attachment 88841 [details]
Addressed feedback.
Comment 9 Dimitri Glazkov (Google) 2011-04-08 10:56:31 PDT
Comment on attachment 88279 [details]
Ready for review.

View in context: https://bugs.webkit.org/attachment.cgi?id=88279&action=review

>> Source/WebCore/dom/EventDispatcher.cpp:174
>> +    bool noCommonBoundary = m_ancestors.isEmpty() || m_ancestors.first().node() == m_ancestors.last().node();
> 
> I'd extract a helper inline function for this.

Done.

>> Source/WebCore/dom/EventDispatcher.cpp:176
>> +    // Build the ancestor chain for relatedTarget.
> 
> I think it's self-evident from the code that you're making a list of ancestors.

Removed the comment.

>> Source/WebCore/dom/EventDispatcher.cpp:221
>> +    }
> 
> I'd extract this as a function.

Done.
Comment 10 Darin Adler 2011-04-08 11:59:41 PDT
Comment on attachment 88841 [details]
Addressed feedback.

View in context: https://bugs.webkit.org/attachment.cgi?id=88841&action=review

> Source/WebCore/dom/EventDispatcher.cpp:128
> +struct BoundaryAdjustments {
> +size_t ancestorSize;
> +Node* relatedTarget;
> +};

Missing indent here.
Comment 11 Dimitri Glazkov (Google) 2011-04-08 12:04:36 PDT
Created attachment 88854 [details]
Adjusted indent
Comment 12 Dimitri Glazkov (Google) 2011-04-08 18:47:52 PDT
I wonder if nice folks will take pity on me and review this patch?
Comment 13 Ojan Vafai 2011-04-08 19:54:02 PDT
Comment on attachment 88854 [details]
Adjusted indent

View in context: https://bugs.webkit.org/attachment.cgi?id=88854&action=review

> LayoutTests/fast/events/shadow-boundary-crossing.html:100
> +        var counter = function()

Nit: functions are usually verbs, no?

> LayoutTests/fast/events/shadow-boundary-crossing.html:112
> +        var count = 0;
> +        var fileInput = document.body.appendChild(document.createElement('input'));
> +        fileInput.setAttribute('type', 'file');
> +        var counter = function()
> +        {
> +            count++;
> +        }
> +        moveOverLeftQuarterOf(fileInput);
> +        document.body.addEventListener('mouseover', counter, false);
> +        document.body.addEventListener('mouseout', counter, false);
> +        moveOverRightQuarterOf(fileInput);
> +        log("The mouseover/mouseout event between two elements inside the same shadow subtree should not propagate out of the shadow DOM", count == 0);
> +        document.body.removeEventListener('mouseover', counter, false);
> +        document.body.removeEventListener('mouseout', counter, false);
> +        fileInput.parentNode.removeChild(fileInput);
> +    },

This would be easier to read with some more line-breaks.

> Source/WebCore/dom/EventDispatcher.cpp:193
> +PassRefPtr<EventTarget> EventDispatcher::adjustRelatedTarget(Event* event, PassRefPtr<EventTarget> relatedTargetArg)

Not sure this name is accurate anymore since this is also adjusting m_ancestors now.

> Source/WebCore/dom/EventDispatcher.cpp:228
> +    BoundaryAdjustments adjustments = adjustToShadowBoundaries(target, m_ancestors, relatedTargetAncestors);
> +    m_ancestors.shrink(adjustments.ancestorSize);
> +    return adjustments.relatedTarget ? adjustments.relatedTarget : relatedTarget;

Can you make adjustToShadowBoundaries a private method? Then, it can adjust m_ancestors directly instead of returning a BoundayAdjustments object. Also, it can then just return the related target or null as approriate.

I think the BoundaryAdjustments struct makes this code a little harder to make sense of.
Comment 14 Ojan Vafai 2011-04-08 19:58:45 PDT
Comment on attachment 88854 [details]
Adjusted indent

View in context: https://bugs.webkit.org/attachment.cgi?id=88854&action=review

> Source/WebCore/dom/EventDispatcher.cpp:161
> +            continue;

this continue isn't needed
Comment 15 Dimitri Glazkov (Google) 2011-04-08 20:00:42 PDT
Comment on attachment 88854 [details]
Adjusted indent

View in context: https://bugs.webkit.org/attachment.cgi?id=88854&action=review

>> LayoutTests/fast/events/shadow-boundary-crossing.html:100
>> +        var counter = function()
> 
> Nit: functions are usually verbs, no?

I am matching the convention elsewhere in this file. Granted, I did write all the tests in this file :)

>> LayoutTests/fast/events/shadow-boundary-crossing.html:112
>> +    },
> 
> This would be easier to read with some more line-breaks.

Sure, I'll add some.

>> Source/WebCore/dom/EventDispatcher.cpp:193
>> +PassRefPtr<EventTarget> EventDispatcher::adjustRelatedTarget(Event* event, PassRefPtr<EventTarget> relatedTargetArg)
> 
> Not sure this name is accurate anymore since this is also adjusting m_ancestors now.

Ancestors is private to EventDispatcher, so the user of the function isn't really aware of what else is happening in the method.

>> Source/WebCore/dom/EventDispatcher.cpp:228
>> +    return adjustments.relatedTarget ? adjustments.relatedTarget : relatedTarget;
> 
> Can you make adjustToShadowBoundaries a private method? Then, it can adjust m_ancestors directly instead of returning a BoundayAdjustments object. Also, it can then just return the related target or null as approriate.
> 
> I think the BoundaryAdjustments struct makes this code a little harder to make sense of.

Sure, will do. I did like the fact that adjustToShadowBoundaries doesn't modify any state, but that's splitting hairs and not very important.
Comment 16 Dimitri Glazkov (Google) 2011-04-08 20:01:37 PDT
Comment on attachment 88854 [details]
Adjusted indent

View in context: https://bugs.webkit.org/attachment.cgi?id=88854&action=review

>> Source/WebCore/dom/EventDispatcher.cpp:161
>> +            continue;
> 
> this continue isn't needed

DUH. Thanks, will remove.
Comment 17 Dimitri Glazkov (Google) 2011-04-09 15:44:53 PDT
I added a diagram at http://trac.webkit.org/export/83382/trunk/Websites/webkit.org/misc/related-target-and-shadow-dom.svg to illustrate what's going on.

For events that have a related target (like mouseover/out), we have to make sure that:

1) if both related target and target are part of a shadow DOM subtree, propagation of the event is confined to that shadow DOM subtree.

2) if the related target is itself inside of a shadow DOM subtree, it is adjusted to the node that host this shadow subtree -- to avoid leaking shadow DOM nodes in event dispatch.

To do this, we must find two boundaries:

1) the lowest common boundary, which is the lowest (in tree sense) possible shadow DOM subtree boundary that includes both related target and target nodes.

2) the first divergent boundary, which is the first shadow DOM boundary that includes related target, but not target.

The code in patch does the following:

1) populate target ancestor chain;
2) populate related target ancestor chain;
3) simultaneously walking both chains from the top, look for divergence (ancestors aren't equal) and keep track of shadow DOM boundaries;
4) once divergence is detected, the last remembered shadow DOM boundary is deemed the lowest common boundary;
5) the walk continues down the related ancestor chain until the next shadow DOM boundary (the first divergent boundary) is found;
6) the target ancestor chain is trimmed up to the lowest common boundary;
7) the related target is changed to the first ancestor outside of the first divergent boundary. 

In addition, there's some logic to deal with edge cases and short-circuiting of the boundary computation in the case when target ancestor chain does cross any shadow DOM boundaries.
Comment 18 Dimitri Glazkov (Google) 2011-04-09 16:30:56 PDT
Created attachment 88935 [details]
More feedback addressed.
Comment 19 Dimitri Glazkov (Google) 2011-04-09 16:31:34 PDT
Feedback addressed, PTAL.
Comment 20 Ojan Vafai 2011-04-09 17:29:42 PDT
Comment on attachment 88935 [details]
More feedback addressed.

View in context: https://bugs.webkit.org/attachment.cgi?id=88935&action=review

> Source/WebCore/dom/EventDispatcher.cpp:128
> +    // Assume divergent boundary is the relatedTarget itself (in other words, related target ancestor chaing does not cross any shadow DOM boundaries).

typo: chaing

> Source/WebCore/dom/EventDispatcher.cpp:154
> +            targetAncestor = m_ancestors.begin();

Do you need to set this? Once diverged == true, we never care about the valur of targetAncestor anymore, right?

> Source/WebCore/dom/EventDispatcher.cpp:170
> +    } else {
> +        // Since ancestors does not contain target itself, we must account
> +        // for the possibility that target is a shadowHost of relatedTarget
> +        // and thus serves as the lowestCommonBoundary.
> +        // Luckily, in this case the firstDivergentBoundary is target.
> +        if ((*firstDivergentBoundary) == m_node.get())
> +            lowestCommonBoundary = m_ancestors.begin();
> +    }

nit: this could be an else-if. your call.
Comment 21 Dimitri Glazkov (Google) 2011-04-09 19:26:33 PDT
Comment on attachment 88935 [details]
More feedback addressed.

View in context: https://bugs.webkit.org/attachment.cgi?id=88935&action=review

Fixing comments and landing now. Thanks Ojan! :)

>> Source/WebCore/dom/EventDispatcher.cpp:128
>> +    // Assume divergent boundary is the relatedTarget itself (in other words, related target ancestor chaing does not cross any shadow DOM boundaries).
> 
> typo: chaing

Doneg.

>> Source/WebCore/dom/EventDispatcher.cpp:154
>> +            targetAncestor = m_ancestors.begin();
> 
> Do you need to set this? Once diverged == true, we never care about the valur of targetAncestor anymore, right?

Oh, that's right! Thanks.

>> Source/WebCore/dom/EventDispatcher.cpp:170
>> +    }
> 
> nit: this could be an else-if. your call.

Ooh, pretty.
Comment 22 Dimitri Glazkov (Google) 2011-04-09 19:33:29 PDT
Committed r83386: <http://trac.webkit.org/changeset/83386>