Bug 91290

When you get a moment, if you could write some sort of test case, that would help me better understand what you're asking me to implement.

It doesn't have to be exotic, just something I can make pass as a v1. Obviously I'll have to write more from the spec myself.

Created attachment 152875 [details] click event test

Created attachment 152876 [details] focus doesn't retarget test

Made a couple clunky testcases. Hopefully it's enough to illustrate. See http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-July/036644.html for how I think this should work and Erik's followup that we shouldn't refire events where target and relatedTarget are in the same document.

Thank you Ojan.

See also http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-July/036647.html. We should probably reuse the event retargeting stuff from web components here.

Thank you, Ojan. Let me think how to implement it. The complexity of event-retargeting spec in Shadow DOM is caused by distributed nodes. Re-targeting events in seamless iframe should not be complex in concept because that does not have such a distributed node. But we should be careful in its implementations. Let me investigate further.

I think we can treat seamless iframe as Web Components conceptually in some cases: A). Non-seamless iframe: No behavior change. Events should stop at iframe boundary definitely. B). Cross-origin seamless iframe: iframe should act like a Web Component. Events should be re-targeted at boundary and propagate to the parent document. C). Same-origin seamless iframe: There should not be boundary at iframe. Events should pass-through the iframe and propagate to parent document. I'd like to avoid adding dependency between WebComponents and seamless iframe spec as much as possible. Let me think further. If you have any opinion, that'd be welcome.

(In reply to comment #9) > I think we can treat seamless iframe as Web Components conceptually in some cases: > > A). Non-seamless iframe: No behavior change. Events should stop at iframe boundary definitely. > > B). Cross-origin seamless iframe: iframe should act like a Web Component. Events should be re-targeted at boundary and propagate to the parent document. > > C). Same-origin seamless iframe: There should not be boundary at iframe. Events should pass-through the iframe and propagate to parent document. > > I'd like to avoid adding dependency between WebComponents and seamless iframe spec as much as possible. Let me think further. If you have any opinion, that'd be welcome. I believe this is conceptually good. I think we should define both the event retargeting algorithm for iframe and web components in spec, since they should be separate. However, they can and should be compatible.

Created attachment 154321 [details] wip. Extends event ancestors across seamless iframes

(In reply to comment #9) > C). Same-origin seamless iframe: There should not be boundary at iframe. Events should pass-through the iframe and propagate to parent document. If we do that you get issues where event.target.ownerDocument changes as you walk up the tree. You also get issues where the prototypes changes as you walk up. This all seems bad to me. We should reduce the multiple global, cross frame access and not add new APIs that depends on this.

(In reply to comment #12) > (In reply to comment #9) > > C). Same-origin seamless iframe: There should not be boundary at iframe. Events should pass-through the iframe and propagate to parent document. > > If we do that you get issues where event.target.ownerDocument changes as you walk up the tree. You also get issues where the prototypes changes as you walk up. This all seems bad to me. We should reduce the multiple global, cross frame access and not add new APIs that depends on this. Thank you for the comment. Although I have not fully understood issues, I am now thinking that we should re-target events on iframes boundary for seamless iframes whether its same origin or not. I won't work on C until we understand every issues we get in C. I'd like to know what are the issues and its worth or not to try to solve using this bug entry.

Let me explain my strategy to implement event-propagation of seamless iframes: 1. Refactor current EventDispatcher so that we can call each event phase (CAPTURE, TARGET, BUBBLING) separately in event dispatching. 2. After that, I'll update the current EventDispatcher::ensureEventAncestor() so that we can pre-calculate every event ancestor nodes across seamless iframes. 3. After that, I'll update EventDispatcher::dispatchEvent (Maybe I'll introduce more *meta* EventDispatcher) so that we can propagate events across iframes. Suppose we have the following HTML and seamless iframes. -- doc.html <A> <B> <iframe id=iframe1 seamless src="iframe1.html"> -- iframe1.html <C> <D> <iframe id=iframe2 seamless src="iframe2.html"> -- iframe2.html <E> <F> And 'click' event is fired on node F. We will dispatch events in the following order: 1. A (capture) 2. B (capture) 3. C (capture) 4. D (capture) 5. E (capture) 6. F (target) 7. E (bubble) 8. iframe2 (target) (target is re-targeted to iframe2) 9. D (bubble) (target: iframe2) 9. C (bubble) (target: iframe2) 10. iframe1 (target) (target is re-targeted to iframe1) 11. B (bubble) (target: iframe1) 12. A (bubble) (target: iframe1) Event objects will be created per Document. Which means node A and node B receive the same event object, but node A and node C don't receive the same event object. Todo: 1. Handle related target. 2. Stop some kinds of events at iframe boundary. I think we can use the same spec from Web Components, but I have to investigate. 3. Consider Security. I appreciate early feedbacks from security guys.

Created attachment 157431 [details] wip. Supports only click and dblclick events

Created attachment 157637 [details] ready for review

Comment on attachment 157637 [details] ready for review Attachment 157637 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13462693

Comment on attachment 157637 [details] ready for review Attachment 157637 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13459831

Comment on attachment 157637 [details] ready for review Attachment 157637 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13463591

Comment on attachment 157637 [details] ready for review Attachment 157637 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13475125

Comment on attachment 157637 [details] ready for review Attachment 157637 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13475130

Comment on attachment 157637 [details] ready for review Attachment 157637 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13458991

Created attachment 157674 [details] Minor update

Comment on attachment 157674 [details] Minor update Attachment 157674 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13471477

Created attachment 157682 [details] fix

Comment on attachment 157682 [details] fix View in context: https://bugs.webkit.org/attachment.cgi?id=157682&action=review The diff is very hard to read, because there's actually just a lot of code being moved around. Any chance of a smaller patch that first simply moves code around to make the actual functional changes easier to review? > Source/WebCore/dom/EventDispatcher.cpp:82 > +class EventDispatcherForBrowsingContext { There's a naming problem here somewhere. The EventDispatcher is actually a manager of a collection of browser-context-specific event dispatcher. Both classes being called EventDispatcher seems odd. > Source/WebCore/dom/EventDispatcher.cpp:478 > + // https://bugs.webkit.org/show_bug.cgi?id=93678 Sounds like the cloning should be a virtual function on an Event, so that then each event could override with the proper cloning implementation.

(In reply to comment #26) > (From update of attachment 157682 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157682&action=review > > The diff is very hard to read, because there's actually just a lot of code being moved around. Any chance of a smaller patch that first simply moves code around to make the actual functional changes easier to review? Okay. That's the weakest point of this patch. Let me try to divide this into smaller patchers. > > > Source/WebCore/dom/EventDispatcher.cpp:82 > > +class EventDispatcherForBrowsingContext { > > There's a naming problem here somewhere. The EventDispatcher is actually a manager of a collection of browser-context-specific event dispatcher. Both classes being called EventDispatcher seems odd. Hmm. I think it is just an implementation detail the EventDispatcher is a manager of a collection of other dispatchers. From the outside, it remains 'EventDispatcher. No change of public interface. Maybe EventDispatcherForBrowsingContext is not a good name, but I don't think it is odd that both are called EventDispatcher. Do you have any suggestions how we call them? I have no better ideas now. > > > Source/WebCore/dom/EventDispatcher.cpp:478 > > + // https://bugs.webkit.org/show_bug.cgi?id=93678 > > Sounds like the cloning should be a virtual function on an Event, so that then each event could override with the proper cloning implementation. Yes, that's on my rader. Let me address this later.

(In reply to comment #27) > Maybe EventDispatcherForBrowsingContext is not a good name, but I don't think it is odd that both are called EventDispatcher. Do you have any suggestions how we call them? I have no better ideas now. PerFrameDispatcher? :)

Sounds good. Shorter is better in this case. (In reply to comment #28) > (In reply to comment #27) > > > Maybe EventDispatcherForBrowsingContext is not a good name, but I don't think it is odd that both are called EventDispatcher. Do you have any suggestions how we call them? I have no better ideas now. > > PerFrameDispatcher? > > :)

Created attachment 158496 [details] wip. sync to ToT

Created attachment 159864 [details] Sync to ToT.

Comment on attachment 159864 [details] Sync to ToT. View in context: https://bugs.webkit.org/attachment.cgi?id=159864&action=review I think you accidentally put up the wrong patch... > Source/WTF/wtf/Assertions.h:402 > +// For ASSERT in both release/debug mode. > +#define MY_ASSERT(assertion) do \ > + if (!(assertion)) { \ > + WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, #assertion); \ > + CRASH(); \ > + } \ > +while (0) What's that? :)

(In reply to comment #32) > (From update of attachment 159864 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159864&action=review > > I think you accidentally put up the wrong patch... Yes, that included the my local debug branch. Let me upload the correct diff. > > > Source/WTF/wtf/Assertions.h:402 > > +// For ASSERT in both release/debug mode. > > +#define MY_ASSERT(assertion) do \ > > + if (!(assertion)) { \ > > + WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, #assertion); \ > > + CRASH(); \ > > + } \ > > +while (0) > > What's that? :)

(In reply to comment #32) > (From update of attachment 159864 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159864&action=review > > I think you accidentally put up the wrong patch... > > > Source/WTF/wtf/Assertions.h:402 > > +// For ASSERT in both release/debug mode. > > +#define MY_ASSERT(assertion) do \ > > + if (!(assertion)) { \ > > + WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, #assertion); \ > > + CRASH(); \ > > + } \ > > +while (0) > > What's that? :) That's for ASSERTION for release build. :)

Created attachment 160049 [details] Sync ToT

Comment on attachment 160049 [details] Sync ToT The plumbing looks mechanically correct. I am still baffled how we let the event trundle through a frame, even if it was destroyed by a listener somewhere earlier in the chain. Ojan, do you have any thoughts on that? Is this what we want?

I agree. We have to be careful about this patch. We don't have to be in a hurry to land this patch. I appreciate opinions from experts, especially from security guys. (In reply to comment #36) > (From update of attachment 160049 [details]) > The plumbing looks mechanically correct. I am still baffled how we let the event trundle through a frame, even if it was destroyed by a listener somewhere earlier in the chain. Ojan, do you have any thoughts on that? Is this what we want?

Created attachment 168672 [details] Sync with ToT

Created attachment 168866 [details] Sync with ToT

Let me resume this work. I think the patch is hard to read since the diff is not clean. If required, it's okay for me to split this patch into several patches, though I have no idea how to split this.

(In reply to comment #40) > Let me resume this work. > > I think the patch is hard to read since the diff is not clean. > If required, it's okay for me to split this patch into several patches, though I have no idea how to split this. It's not clear that this patch is in anyone's hotlist. Can we prioritize it lower than other, more pressing work?

Comment on attachment 168866 [details] Sync with ToT Is there a way to do this in smaller pieces so the behavior changing part is separate from the refactoring?

(In reply to comment #40) > If required, it's okay for me to split this patch into several patches, though I have no idea how to split this. I strongly encourage you do it. It should be straightforward to split it since it’s largely refactoring.

(In reply to comment #43) > (In reply to comment #40) > > If required, it's okay for me to split this patch into several patches, though I have no idea how to split this. > > I strongly encourage you do it. It should be straightforward to split it since it’s largely refactoring. Also, the approach could be simplified, I think: 1) each ancestor chain holds a refptr to parent iframe (which keeps the whole chain in place). 2) when creating a list of ancestors, we store the ptr there 3) and hand off event dispatch to parent frame. 4) after dispatch completes, we call to clear parent frame's ptr and clear our own. No need to create a new layer of indirection in dispatcher.

Clearing a review flag. I don't have a plan to continue this. Please feel free to grab this bug if someone is interested in this.

seamless iframe support was removed in this commit: https://github.com/WebKit/WebKit/commit/3e2010a853456a8ad1e097897ef35ff5b54b2877 I am going to mark this and dependent open bug as "RESOLVED WONTFIX"? Thanks!