Route guest frame unload notifications via host frame #3812

robertknight · 2021-10-07T10:20:19Z

Depends on #3807

Change how the sidebar is notified of guest frames being unloaded to
support guest frames where the client has been loaded via means other
than HypothesisInjector or where the guest is cross-origin.

Instead of listening for the guest frame's 'unload' event from the
parent frame in HypothesisInjector, the guest frame instead listens
for this event itself and sends a hypothesisGuestUnloaded message to
the host frame via window.postMessage, which in turn is handled in the
Sidebar class to relay it to the sidebar app via a destroyFrame RPC
call. This indirect route works around a bug in Safari (see code
comments).

As well as supporting future use cases, this also simplifies the
HypothesisInjector class as it no longer needs access to the Bridge.

To facilitate testing, I changed the http://localhost:3000/document/parent-frame page to add a "Toggle frame" button that removes or adds the iframe to the page.

Testing:

Go to http://localhost:3000/document/parent-frame
Add annotations to the iframe
Click the Toggle button to remove the frame from the page, then again to re-add it

When the frame is removed the annotations should disappear from the sidebar, and when they are re-added, they should re-appear again. They may show up as Orphans when loaded, but this is due to an existing issue unrelated to these changes.

Additionally if you combine this branch with #3811, you'll see the list of URLs in the Help panel's "About this version" tab update to reflect the connected frames when you toggle the presence of the guest frame.

Implement the `injectClient` method of Guest that is used by integrations (eg. VitalSource) to inject the client into a chosen frame. In the process the `HypothesisInjector` class has been extracted out of the `CrossFrame` class, since it is unrelated to the rest of the functionality in that class and only lived their because of its dependence on the `Bridge` instance, which will soon be removed (see #3812). It is now constructed and used directly by the `Guest` class instead.

esanzgar

In this PR, there is an implicit assumption that a host frame is the direct parent of the guest frame. However, the guest frame could be a descendant of the host (child, grand-child, etc..).

I think one way the above issue while still supporting cross-origin guest frames is to pass a configuration that tells the position from the host frame:

subframeIdentifier: ....,
hostFrame: 3

which will indicate that the host frame is three levels up: window.parent.parent.parent.

esanzgar · 2021-10-07T14:02:31Z

src/annotator/index.js

@@ -51,15 +51,14 @@ function init() {
  window_.__hypothesis = {};

  const annotatorConfig = getConfig('annotator');
+  const hostFrame = annotatorConfig.subFrameIdentifier ? window.parent : window;


The host frame could be more than one level up: window.parent.parent..... I think you need to look up until you have a window without the subFrameIdentifier . The problem with that is that only works if the parent window is same-origin.

esanzgar · 2021-10-07T14:07:10Z

src/annotator/index.js

  let sidebar;
-  if (!annotatorConfig.subFrameIdentifier) {
+  if (window === hostFrame) {


I prefer the previous version than this indirection.

esanzgar · 2021-10-07T14:09:45Z

src/annotator/sidebar.js

+    // Notify sidebar when a guest is unloaded. This message is routed via
+    // the host frame because in Safari guest frames are unable to send messages
+    // directly to the sidebar during a window's 'unload' event.
+    // See https://bugs.webkit.org/show_bug.cgi?id=231167.


This is really annoying bug. Because of that we need to make all these changes.

esanzgar · 2021-10-07T14:10:42Z

src/annotator/sidebar.js

+    // directly to the sidebar during a window's 'unload' event.
+    // See https://bugs.webkit.org/show_bug.cgi?id=231167.
+    this._listeners.add(window, 'message', event => {
+      const messageData = /** @type {MessageEvent} */ (event).data;


Suggested change

const messageData = /** @type {MessageEvent} */ (event).data;

const {data} = /** @type {MessageEvent} */ (event);

It is possible (albeit a bit pointless) that some code in the host page may send a message event with a data value that is null or undefined. For this reason we don't destructure here.

I think {data: null} is a valid MessageEvent, but data property is always defined.

Oh, indeed, you're right. I'll make this change then.

robertknight · 2021-10-07T14:45:24Z

In this PR, there is an implicit assumption that a host frame is the direct parent of the guest frame. However, the guest frame could be a descendant of the host (child, grand-child, etc..).

You're right there is this assumption. Currently all the use cases we have (ie. ebook readers) only involve guest frames that are direct children of the host. There is also some existing code in annotator/index.js that makes this assumption (eg. the guest <-> sidebar communication).

I think your proposal to add something to the config to indicate how many levels up the tree the host is makes sense as a future addition, but I'd like to keep the direct-child assumption in this PR.

This enables testing handling of the client's discovery of existing frames (on page load) as well as handling of dynamic addition and removal after the client has started.

Change how the sidebar is notified of guest frames being unloaded to support guest frames where the client has been loaded via means other than `HypothesisInjector` or where the guest is cross-origin. Instead of listening for the guest frame's 'unload' event from the parent frame in `HypothesisInjector`, the guest frame instead listens for this event itself and sends a `hypothesisGuestUnloaded` message to the host frame via `window.postMessage`, which in turn is handled in the `Sidebar` class to relay it to the sidebar app via a `destroyFrame` RPC call. This indirect route works around a bug in Safari (see code comments). As well as supporting future use cases, this also simplifies the `HypothesisInjector` class as it no longer needs access to the `Bridge`.

Implement the `injectClient` method of Guest that is used by integrations (eg. VitalSource) to inject the client into a chosen frame. In the process the `HypothesisInjector` class has been extracted out of the `CrossFrame` class, since it is unrelated to the rest of the functionality in that class and only lived their because of its dependence on the `Bridge` instance, which will soon be removed (see #3812). It is now constructed and used directly by the `Guest` class instead.

robertknight requested a review from esanzgar October 7, 2021 10:20

robertknight added the VitalSource label Oct 7, 2021

robertknight mentioned this pull request Oct 7, 2021

Implement "manual" injection of client into frames when requested by integration #3813

Merged

esanzgar suggested changes Oct 7, 2021

View reviewed changes

esanzgar approved these changes Oct 7, 2021

View reviewed changes

Base automatically changed from split-guest-host-messages to master October 7, 2021 15:43

robertknight added 3 commits October 7, 2021 16:45

Add toggle button to "Annotatable iframe" test page

30c39c6

This enables testing handling of the client's discovery of existing frames (on page load) as well as handling of dynamic addition and removal after the client has started.

Use destructuring to make some code more succinct

e6af3ec

robertknight force-pushed the notify-sidebar-on-frame-unload branch from 189dcfa to e6af3ec Compare October 7, 2021 15:48

robertknight merged commit 05b6a93 into master Oct 7, 2021

robertknight deleted the notify-sidebar-on-frame-unload branch October 7, 2021 15:53

robertknight mentioned this pull request Oct 14, 2021

Issues with guest frame support that impact VitalSource #3798

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Route guest frame unload notifications via host frame #3812

Route guest frame unload notifications via host frame #3812

robertknight commented Oct 7, 2021

esanzgar left a comment •

edited

Loading

esanzgar Oct 7, 2021

esanzgar Oct 7, 2021

esanzgar Oct 7, 2021

esanzgar Oct 7, 2021

robertknight Oct 7, 2021

esanzgar Oct 7, 2021

robertknight Oct 7, 2021

robertknight commented Oct 7, 2021

	const messageData = /** @type {MessageEvent} */ (event).data;
	const {data} = /** @type {MessageEvent} */ (event);

Route guest frame unload notifications via host frame #3812

Route guest frame unload notifications via host frame #3812

Conversation

robertknight commented Oct 7, 2021

esanzgar left a comment • edited Loading

Choose a reason for hiding this comment

esanzgar Oct 7, 2021

Choose a reason for hiding this comment

esanzgar Oct 7, 2021

Choose a reason for hiding this comment

esanzgar Oct 7, 2021

Choose a reason for hiding this comment

esanzgar Oct 7, 2021

Choose a reason for hiding this comment

robertknight Oct 7, 2021

Choose a reason for hiding this comment

esanzgar Oct 7, 2021

Choose a reason for hiding this comment

robertknight Oct 7, 2021

Choose a reason for hiding this comment

robertknight commented Oct 7, 2021

esanzgar left a comment •

edited

Loading