Bug 55515

Created attachment 84307 [details] Patch

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.

One solution is to move retargeting into the event. Let me see how that works.

Comment on attachment 84307 [details] Patch removing r? flag for now. Refactoring event code..

Created attachment 87975 [details] Using EventDispatcher.

Created attachment 88279 [details] Ready for review.

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.

Created attachment 88841 [details] Addressed feedback.

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 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.

Created attachment 88854 [details] Adjusted indent

I wonder if nice folks will take pity on me and review this patch?

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 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 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 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.

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.

Created attachment 88935 [details] More feedback addressed.

Feedback addressed, PTAL.

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 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.

Committed r83386: <http://trac.webkit.org/changeset/83386>