Bug 91290 - Support an event propagation across seamless iframes.
Summary: Support an event propagation across seamless iframes.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 92613 92614 92621 92625 93322 93452 93678 93696 93959
Blocks: 45950
  Show dependency treegraph
 
Reported: 2012-07-13 15:11 PDT by Ojan Vafai
Modified: 2017-07-18 08:29 PDT (History)
14 users (show)

See Also:


Attachments
click event test (879 bytes, text/html)
2012-07-17 16:54 PDT, Ojan Vafai
no flags Details
focus doesn't retarget test (793 bytes, text/html)
2012-07-17 16:55 PDT, Ojan Vafai
no flags Details
wip. Extends event ancestors across seamless iframes (4.78 KB, patch)
2012-07-25 04:14 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
wip. Supports only click and dblclick events (30.10 KB, patch)
2012-08-09 04:50 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
ready for review (34.26 KB, patch)
2012-08-09 22:14 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
Minor update (38.15 KB, patch)
2012-08-10 01:40 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
fix (38.15 KB, patch)
2012-08-10 02:10 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
wip. sync to ToT (40.68 KB, patch)
2012-08-14 21:21 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
Sync to ToT. (44.85 KB, patch)
2012-08-21 23:13 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
Sync ToT (37.01 KB, patch)
2012-08-22 17:43 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
Sync with ToT (36.71 KB, patch)
2012-10-15 04:02 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
Sync with ToT (36.59 KB, patch)
2012-10-15 23:28 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2012-07-13 15:11:48 PDT
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.
Comment 1 Eric Seidel (no email) 2012-07-13 16:31:57 PDT
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.
Comment 2 Eric Seidel (no email) 2012-07-13 16:32:23 PDT
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.
Comment 3 Ojan Vafai 2012-07-17 16:54:31 PDT
Created attachment 152875 [details]
click event test
Comment 4 Ojan Vafai 2012-07-17 16:55:41 PDT
Created attachment 152876 [details]
focus doesn't retarget test
Comment 5 Ojan Vafai 2012-07-17 16:57:23 PDT
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.
Comment 6 Eric Seidel (no email) 2012-07-17 22:43:48 PDT
Thank you Ojan.
Comment 7 Ojan Vafai 2012-07-18 08:37:35 PDT
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.
Comment 8 Hayato Ito 2012-07-19 01:58:12 PDT
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.
Comment 9 Hayato Ito 2012-07-24 23:51:55 PDT
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.
Comment 10 Shinya Kawanaka 2012-07-24 23:56:33 PDT
(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.
Comment 11 Hayato Ito 2012-07-25 04:14:10 PDT
Created attachment 154321 [details]
wip. Extends event ancestors across seamless iframes
Comment 12 Erik Arvidsson 2012-07-25 11:57:04 PDT
(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.
Comment 13 Hayato Ito 2012-07-25 19:52:19 PDT
(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.
Comment 14 Hayato Ito 2012-07-29 23:30:12 PDT
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.
Comment 15 Hayato Ito 2012-08-09 04:50:23 PDT
Created attachment 157431 [details]
wip. Supports only click and dblclick events
Comment 16 Hayato Ito 2012-08-09 22:14:25 PDT
Created attachment 157637 [details]
ready for review
Comment 17 Build Bot 2012-08-09 22:32:47 PDT
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 18 WebKit Review Bot 2012-08-09 22:41:44 PDT
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 19 Early Warning System Bot 2012-08-09 22:45:42 PDT
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 20 Build Bot 2012-08-09 22:57:24 PDT
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 21 Early Warning System Bot 2012-08-09 23:17:21 PDT
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 22 Gyuyoung Kim 2012-08-09 23:32:47 PDT
Comment on attachment 157637 [details]
ready for review

Attachment 157637 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13458991
Comment 23 Hayato Ito 2012-08-10 01:40:51 PDT
Created attachment 157674 [details]
Minor update
Comment 24 WebKit Review Bot 2012-08-10 02:04:35 PDT
Comment on attachment 157674 [details]
Minor update

Attachment 157674 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13471477
Comment 25 Hayato Ito 2012-08-10 02:10:09 PDT
Created attachment 157682 [details]
fix
Comment 26 Dimitri Glazkov (Google) 2012-08-10 09:10:15 PDT
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.
Comment 27 Hayato Ito 2012-08-12 20:54:46 PDT
(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.
Comment 28 Dimitri Glazkov (Google) 2012-08-13 11:10:31 PDT
(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?

:)
Comment 29 Hayato Ito 2012-08-14 05:19:27 PDT
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?
> 
> :)
Comment 30 Hayato Ito 2012-08-14 21:21:09 PDT
Created attachment 158496 [details]
wip. sync to ToT
Comment 31 Hayato Ito 2012-08-21 23:13:28 PDT
Created attachment 159864 [details]
Sync to ToT.
Comment 32 Dimitri Glazkov (Google) 2012-08-22 15:22:38 PDT
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? :)
Comment 33 Hayato Ito 2012-08-22 15:29:22 PDT
(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? :)
Comment 34 Hayato Ito 2012-08-22 15:38:31 PDT
(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. :)
Comment 35 Hayato Ito 2012-08-22 17:43:49 PDT
Created attachment 160049 [details]
Sync ToT
Comment 36 Dimitri Glazkov (Google) 2012-08-22 20:31:21 PDT
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?
Comment 37 Hayato Ito 2012-08-22 20:36:53 PDT
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?
Comment 38 Hayato Ito 2012-10-15 04:02:21 PDT
Created attachment 168672 [details]
Sync with ToT
Comment 39 Hayato Ito 2012-10-15 23:28:20 PDT
Created attachment 168866 [details]
Sync with ToT
Comment 40 Hayato Ito 2012-10-15 23:31:42 PDT
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.
Comment 41 Dimitri Glazkov (Google) 2012-10-16 07:36:41 PDT
(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 42 Darin Adler 2012-10-16 17:55:13 PDT
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?
Comment 43 Darin Adler 2012-10-16 17:56:04 PDT
(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.
Comment 44 Dimitri Glazkov (Google) 2012-10-16 17:59:05 PDT
(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.
Comment 45 Hayato Ito 2013-04-08 20:29:25 PDT
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.