Explicitly specify guest/sidebar frames to ping in `startDiscovery` #3599

robertknight · 2021-07-21T09:59:47Z

The logic used by guest/host and sidebar frames to find each other in
Discovery#startDiscovery relied on traversing the frame tree starting
from window.top using window.frames. Since window.frames doesn't
include frames in shadow roots, this did not work if both the guest and
sidebar are contained within a shadow root. The sidebar is always
contained in a shadow root created by the <hypothesis-sidebar>
element, so this simplifies to only working if the guest/host is not
contained in a shadow root.

For current use cases it is possible for guest frames to get a direct
reference to the sidebar frame they should be communicating with and for
the sidebar to get direct a reference to the host frame (its parent).
This avoids the need for window.frames traversal.

This commit leverages that by changing startDiscovery to take an
explicit array of frames to ping. Guest frames invoke startDiscovery
passing a reference to the sidebar frame and sidebar frames invoke
startDiscovery passing a reference to the parent (host) frame.

This fixes the following scenarios:

Hypothesis not completing loading if host frame is contained within a
shadow root. eg. In the VitalSource book reader. See http://localhost:3000/document/host-in-shadow-root test case in client dev server.
Same-origin guest frames loading Hypothesis after the sidebar has
already loaded. See http://localhost:3000/document/parent-frame test
case in client dev server.
Multiple sidebars attempting to connect to the same guest frame, if
Hypothesis is loaded multiple times in different parts of the frame
tree. See http://localhost:3000/document/multi-frames test case in
client dev server.

This approach has some limitations:

Child guest frames which are not same-origin with the parent host frame
are not supported.
Child guest frames which attempt to connect to the sidebar before it
has loaded will fail to connect. This could be remedied by making the
guest wait for confirmation of the sidebar having loaded before
calling connectToSidebar. This doesn't affect the host frame since
the sidebar pings that frame directly.

These limitations are acceptable in the short term but will be remedied
by a larger overhaul of inter-frame communication that is currently
being worked on.

Fixes #3590
This is also a short-term fix for #249 and #187. A better solution will be coming with #3533.

Add a test case to the client's dev server which reproduces the issue from the VitalSource reader (possibly as separate PR) (Add manual test case for when Hypothesis is loaded into an iframe inside a shadow root #3600)
Update the tests for Discovery, CrossFrame, FrameSync

lyzadanger

Hokay! Once one gets one's head around this architecture, it makes general sense. There's still the moderately-steep cognitive investment required for a developer to get up to speed with how these pieces play together; I'm hoping that is something that will be paid down a bit with continued improvement in these areas.

The net effect isn't too complicated, I suppose, which is:

host frame will create the sidebar frame and stick it in a global reference available to other frames (with origin restrictions, etc.)
any additional guest frame will look for that global sidebar-window reference and attempt to establish communication with it.

This differs from the past implementation in which the guest frames didn't do anything, but the discovery service would attempt to iterate through descendant frames and establish connections between them and the (I think?) sidebar. This had a shortcoming because the list of frames available to that discovery service didn't include shadow-DOM-ed frames.

(I know I'm just re-saying what you already said, but I want to be sure I've got it right!)

Given that, the approach seems OK as far as I can tell.

Most of my comments here are around places that commenting could help out in the interim.

lyzadanger · 2021-07-21T18:16:12Z

src/annotator/cross-frame.js

+    /**
+     * Attempt to connect to the sidebar frame.
+     *
+     * @param {Window} frame


Can you expand this to explain which frame we're talking about? I presume it's a given guest frame that wants to find a sidebar?

lyzadanger · 2021-07-21T18:17:03Z

src/annotator/index.js

@@ -34,32 +34,49 @@ const appLinkEl = /** @type {Element} */ (
 );

 function init() {
+  window_.__hypothesis = {};


Can this be typed/documented?

src/annotator/index.js

lyzadanger · 2021-07-21T18:35:17Z

src/annotator/index.js

  const eventBus = new EventBus();
  const guest = new Guest(document.body, eventBus, {
    ...annotatorConfig,
    // Load the PDF anchoring/metadata integration.
    // nb. documentType is an internal config property only
    documentType: isPDF ? 'pdf' : 'html',
  });
+


Perhaps a comment explaining that the host will create the sidebar, while the guest-only frames (which have a subFrameIdentifier)...don't. Anything to help reduce the magic-ness of subFrameIdentifier here would be helpful!

lyzadanger · 2021-07-21T18:36:55Z

src/annotator/index.js

  // Clear `annotations` value from the notebook's config to prevent direct-linked
  // annotations from filtering the threads.
  const notebook = new Notebook(document.body, eventBus, getConfig('notebook'));

+  // Setup guest <-> sidebar communication.


Suggested change

// Setup guest <-> sidebar communication.

// Set up communication between this window/frame (guest) and the sidebar frame

Or similar?

lyzadanger · 2021-07-21T18:37:37Z

src/annotator/index.js

+    : /** @type {HypothesisWindow} */ (window.parent).__hypothesis
+        ?.sidebarWindow;
+  if (sidebarWindow) {
+    guest.crossframe.connectToSidebar(sidebarWindow);


I appreciate that this is a separate step from CrossFrame object instantiation—it feels right 👍🏻

lyzadanger · 2021-07-21T18:38:55Z

src/types/annotator.js

- *   Flag used to indicate that the "annotator" part of Hypothesis is loaded in
- *   the current frame.
+ * @prop {object} [__hypothesis] - Internal data related to supporting guests in iframes
+ *   @prop {Window} [sidebarWindow] - The sidebar window that is active in this frame.


What does "this frame" mean, exactly, here?

lyzadanger · 2021-07-21T18:39:57Z

src/sidebar/services/frame-sync.js

+      (source, origin, token) =>
+        this._bridge.createChannel({ source, origin, token }),
+      [
+        // Ping the host frame which is in most cases also the only guest.


Can you help me understand why it's relevant here that the host frame is usually the only guest? Is it assured that the parent frame is definitely the host?

lyzadanger · 2021-07-21T18:51:33Z

src/shared/discovery.js

+
+    // Ping specified other frames to tell them about the existence of this frame.
+    const beaconMessage = this.server
+      ? '__cross_frame_dhcp_offer'


Feel free to tell me to take a hike because this couldn't be more tangential, but I'm curious as to the meaning of dhcp in this context. Is it meaningful? DCHP as I (don't really) understand it seems pretty specific. Again, just a curiosity question.

codecov · 2021-07-22T11:49:34Z

Codecov Report

Merging #3599 (0f70af2) into master (5ca9d39) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3599      +/-   ##
==========================================
+ Coverage   98.60%   98.61%   +0.01%     
==========================================
  Files         211      211              
  Lines        7738     7735       -3     
  Branches     1758     1758              
==========================================
- Hits         7630     7628       -2     
+ Misses        108      107       -1

Impacted Files	Coverage Δ
src/annotator/sidebar.js	`98.04% <ø> (ø)`
src/annotator/cross-frame.js	`100.00% <100.00%> (ø)`
src/annotator/util/frame-util.js	`83.33% <100.00%> (ø)`
src/shared/discovery.js	`93.44% <100.00%> (-0.68%)`	⬇️
src/sidebar/services/frame-sync.js	`100.00% <100.00%> (+1.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ca9d39...0f70af2. Read the comment docs.

robertknight · 2021-07-22T12:56:13Z

I have updated this PR to add tests and also some additional documentation per PR feedback. Some of the issues resolved in this PR can be tested with the existing test cases mentioned in the PR description. If you create a temporary branch which merges this branch with #3600 you can also test out the fix that should resolve the issue in the new VitalSource reader. In this temporary branch if you visit http://localhost:3000/document/host-in-shadow-root you should see that the sidebar now appears inside the iframe, whereas it doesn't on master.. Edit: #3600 has now been merged into this branch, so you can just visithttp://localhost:3000/document/host-in-shadow-root .

The logic used by guest/host and sidebar frames to find each other in `Discovery#startDiscovery` relied on traversing the frame tree starting from `window.top` using `window.frames`. Since `window.frames` doesn't include frames in shadow roots, this did not work if _both_ the guest and sidebar are contained within a shadow root. The sidebar is always contained in a shadow root created by the `<hypothesis-sidebar>` element, so this simplifies to only working if the guest/host is not contained in a shadow root. For current use cases it is possible for guest frames to get a direct reference to the sidebar frame they should be communicating with and for the sidebar to get direct a reference to the host frame (its parent). This avoids the need for `window.frames` traversal. This commit leverages that by changing `startDiscovery` to take an explicit array of frames to ping. Guest frames invoke `startDiscovery` passing a reference to the sidebar frame and sidebar frames invoke `startDiscovery` passing a reference to the parent (host) frame. This fixes the following scenarios: - Hypothesis not completing loading if host frame is contained within a shadow root. eg. In the VitalSource book reader. - Same-origin guest frames loading Hypothesis after the sidebar has already loaded. See http://localhost:3000/document/parent-frame test case in client dev server. - Multiple sidebars attempting to connect to the same guest frame, if Hypothesis is loaded multiple times in different parts of the frame tree. See http://localhost:3000/document/multi-frames test case in client dev server. This approach has some limitations: - Child guest frames which are not same-origin with the parent host frame are not supported. - Child guest frames which attempt to connect to the sidebar before it has loaded will fail to connect. This could be remedied by making the guest wait for confirmation of the sidebar having loaded before calling `connectToSidebar`. This doesn't affect the host frame since the sidebar pings that frame directly. These limitations are acceptable in the short term but will be remedied by a larger overhaul of inter-frame communication that is currently being worked on.

Add comments to clarify various aspects of the way the host/guest frames are set up and interact.

Rather than use a dedicated getter to expose the sidebar frame, it seemed better on a second look to use the existing `sidebar.iframe` property that exposes the `<iframe>` element, but document and test that property.

lyzadanger

I have verified locally that these changes cause the "manual test case" document in the dev server to work as expected.

Unrelated to these changes, the FrameSync test fixtures, fakes and setup are complex enough that I don't feel confident in being able to guarantee the "whole picture," but the changes you made within them make sense. (My point here is that the pre-existing tests have a bit of a complexity-smell; nothing to address right now).

src/annotator/index.js

lyzadanger · 2021-07-22T15:44:00Z

src/sidebar/services/test/frame-sync-test.js

+    fakeDiscovery = {
+      startDiscovery: sinon.stub(),
+    };
+    const FakeDiscovery = sinon.stub().returns(fakeDiscovery);


Thanks. This is more consistent with our test conventions.

lyzadanger · 2021-07-22T15:49:16Z

src/sidebar/services/test/frame-sync-test.js

  });

  afterEach(() => {
    $imports.$restore();
  });

+  describe('#connect', () => {
+    it('establishes a connection to the parent host/guest frame', () => {


This is splitting hairs, but it doesn't have any knowledge about the host-guest-iness of the parent frame; it's merely establishing a connection with whatever the parent frame is.

robertknight force-pushed the direct-guest-sidebar-connection-v2 branch from e27c39b to f9bbf67 Compare July 21, 2021 10:22

lyzadanger reviewed Jul 21, 2021

View reviewed changes

robertknight force-pushed the direct-guest-sidebar-connection-v2 branch from f9bbf67 to eece43e Compare July 22, 2021 11:46

robertknight added 3 commits July 22, 2021 14:25

Add additional comments and revise logic slightly in annotator/index.js

ddcd152

Add comments to clarify various aspects of the way the host/guest frames are set up and interact.

Replace sidebarWindow property with sidebar.iframe.contentWindow

0f70af2

Rather than use a dedicated getter to expose the sidebar frame, it seemed better on a second look to use the existing `sidebar.iframe` property that exposes the `<iframe>` element, but document and test that property.

robertknight force-pushed the direct-guest-sidebar-connection-v2 branch from eece43e to 0f70af2 Compare July 22, 2021 13:25

robertknight marked this pull request as ready for review July 22, 2021 13:27

lyzadanger approved these changes Jul 22, 2021

View reviewed changes

robertknight merged commit 4305f25 into master Jul 25, 2021

robertknight deleted the direct-guest-sidebar-connection-v2 branch July 25, 2021 16:08

This was referenced Jul 25, 2021

Get Hypothesis client working in new VitalSource reader #3590

Closed

Multiple unrelated Hypothesis frames on one page causes havoc #249

Closed

Error when top level frame and iframe on a page embed the Hypothesis client #187

Closed

robertknight mentioned this pull request Jul 29, 2021

Epub.js demo does not work #4283

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explicitly specify guest/sidebar frames to ping in `startDiscovery` #3599

Explicitly specify guest/sidebar frames to ping in `startDiscovery` #3599

robertknight commented Jul 21, 2021 •

edited

Loading

lyzadanger left a comment

lyzadanger Jul 21, 2021

lyzadanger Jul 21, 2021

lyzadanger Jul 21, 2021

lyzadanger Jul 21, 2021

lyzadanger Jul 21, 2021

lyzadanger Jul 21, 2021

lyzadanger Jul 21, 2021

lyzadanger Jul 21, 2021

codecov bot commented Jul 22, 2021 •

edited

Loading

robertknight commented Jul 22, 2021 •

edited

Loading

lyzadanger left a comment

lyzadanger Jul 22, 2021

lyzadanger Jul 22, 2021

	// Setup guest <-> sidebar communication.
	// Set up communication between this window/frame (guest) and the sidebar frame

Explicitly specify guest/sidebar frames to ping in startDiscovery #3599

Explicitly specify guest/sidebar frames to ping in startDiscovery #3599

Conversation

robertknight commented Jul 21, 2021 • edited Loading

lyzadanger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 22, 2021 • edited Loading

Codecov Report

robertknight commented Jul 22, 2021 • edited Loading

lyzadanger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Explicitly specify guest/sidebar frames to ping in `startDiscovery` #3599

Explicitly specify guest/sidebar frames to ping in `startDiscovery` #3599

robertknight commented Jul 21, 2021 •

edited

Loading

codecov bot commented Jul 22, 2021 •

edited

Loading

robertknight commented Jul 22, 2021 •

edited

Loading