Bug 91290 – Support an event propagation across seamless iframes.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Layout and Rendering

[Ojan Vafai]

See http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-July/036584.html. Off-thread, it sounds like Hixie is fine with this conceptually and will spec whatever we implement.

We should implement this using http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#event-retargeting. The only thing we need to add to that is that mouse events need to have their coordinate space appropriately changed when crossing the document boundary.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Ojan Vafai]

Created attachment 152875 [details]
click event test

[Ojan Vafai]

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

[Ojan Vafai]

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.

[Eric Seidel (no email)]

Thank you Ojan.

[Ojan Vafai]

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.

[Hayato Ito]

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.

[Hayato Ito]

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.

[Shinya Kawanaka]

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

[Hayato Ito]

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

[Erik Arvidsson]

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

[Hayato Ito]

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

[Hayato Ito]

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.

[Hayato Ito]

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

[Hayato Ito]

Created attachment 157637 [details]
ready for review

[Build Bot]

Comment on attachment 157637 [details]
ready for review

Attachment 157637 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13462693

[WebKit Review Bot]

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

[Early Warning System Bot]

Comment on attachment 157637 [details]
ready for review

Attachment 157637 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13463591

[Build Bot]

Comment on attachment 157637 [details]
ready for review

Attachment 157637 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13475125

[Early Warning System Bot]

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

[Gyuyoung Kim]

Comment on attachment 157637 [details]
ready for review

Attachment 157637 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13458991

[Hayato Ito]

Created attachment 157674 [details]
Minor update

[WebKit Review Bot]

Comment on attachment 157674 [details]
Minor update

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

[Hayato Ito]

Created attachment 157682 [details]
fix

[Dimitri Glazkov (Google)]

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.

[Hayato Ito]

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

[Dimitri Glazkov (Google)]

(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?

:)

[Hayato Ito]

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?
> 
> :)

[Hayato Ito]

Created attachment 158496 [details]
wip. sync to ToT

[Hayato Ito]

Created attachment 159864 [details]
Sync to ToT.

[Dimitri Glazkov (Google)]

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? :)

[Hayato Ito]

(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? :)

[Hayato Ito]

(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. :)

[Hayato Ito]

Created attachment 160049 [details]
Sync ToT

[Dimitri Glazkov (Google)]

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?

[Hayato Ito]

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?

[Hayato Ito]

Created attachment 168672 [details]
Sync with ToT

[Hayato Ito]

Created attachment 168866 [details]
Sync with ToT

[Hayato Ito]

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.

[Dimitri Glazkov (Google)]

(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?

[Darin Adler]

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?

[Darin Adler]

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

[Dimitri Glazkov (Google)]

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

[Hayato Ito]

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.

[Ahmad Saleem]

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!