Only send annotations to matching frame #4131

robertknight · 2022-01-21T14:14:28Z

Depends on #4129

When the sidebar is connected to multiple guest frames it will send all
incoming annotations to all frames. The result is typically that the
annotation will anchor in one frame and orphan in the others. Depending
on what order this happens in, the annotation will non-deterministically
show up as an Annotation or Orphan in the sidebar.

In order to determine which frames an annotation should be sent to in
all cases, we'd either need the backend to return information about which
search URIs an annotation matches or make a separate search request for
each frame and record the associated frame with the results. This
will require some significant refactoring of the annotation search
service.

As an interim step, make FrameSyncService send annotations only to a
single frame based on matching URL, with a fallback to sending to the
main frame if there is no exact match. This will work as expected for
most pages, and is at least deterministic when it does fail. When we
have a solution for being able to match annotations to frames more
generally, we can adapt this code to use it.

This is a partial solution to #3992.

TODO:

FrameSyncService needs some additional tests to cover the new behaviors.
Test this with VitalSource using the browser extension

Testing:

In one of the dev server test pages that contains an annotation-enabled iframe, try annotating the host frame (if possible) and the iframe, and then reload the page several times.

On master, annotations may sometimes appear in the "Orphans" tab. On this branch, all the annotations should consistently anchor in the expected frame.

codecov · 2022-01-21T14:15:20Z

Codecov Report

Merging #4131 (989be8c) into master (9465de9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4131   +/-   ##
=======================================
  Coverage   99.08%   99.09%           
=======================================
  Files         219      219           
  Lines        8229     8249   +20     
  Branches     1919     1923    +4     
=======================================
+ Hits         8154     8174   +20     
  Misses         75       75

Impacted Files	Coverage Δ
src/sidebar/services/frame-sync.js	`100.00% <100.00%> (ø)`

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 9465de9...989be8c. Read the comment docs.

robertknight · 2022-01-21T14:59:21Z

I'm seeing an issue when testing this branch with VitalSource using the browser extension. The guest frame appears to be receiving an unload just after the guest has loaded and connected to the sidebar. This results in the guest being removed from the sidebar immediately after it is added.

robertknight · 2022-01-24T12:10:23Z

I'm seeing an issue when testing this branch with VitalSource using the browser extension. The guest frame appears to be receiving an unload just after the guest has loaded and connected to the sidebar

It looks like what is happening here is that chapter content loads in several phases:

The iframe for the previous chapter is removed
An iframe is created for the new chapter and navigated to an initial URL. For example https://jigsaw.vitalsource.com/books/9781319367077/epub/OEBPS/xhtml/lun_9781319056261_ch01_04.xhtml?brand=vitalsource&create=true
The iframe created in step 2 is then navigated to a new URL via a form submission. The form action URL has the same URL as step 2 but with favre=brett query params. For example https://jigsaw.vitalsource.com/books/9781319367077/epub/OEBPS/xhtml/lun_9781319056261_ch01_04.xhtml?brand=vitalsource&create=true&favre=brett.

If the client gets injected into the new frame after step 2, then it gets unloaded in step 3 and doesn't get re-injected, because the VitalSource integration only monitors for creation of iframes, not navigation of existing iframes.

robertknight · 2022-01-27T13:44:28Z

src/sidebar/services/frame-sync.js

+    let frameIdentifier = /** @type {string|null} */ (
+      `temp-${this._nextGuestId}`
+    );
+    this._guestRPC.set(frameIdentifier, guestRPC);


In this PR I have not changed how frame IDs are allocated. If we change this in future so that frame IDs are either allocated entirely within the sidebar or in such a way that we can know them as soon as the guest connects, then we can simplify this code.

In the future, we could easily add a capability to PortFinder#discover to send a frame identifier.

Alternatively, if gathering the information for the documentInfoChanged event is slow, the frame identifier could be send using a new event (decoupled from documentInfoChanged). The frame identifier that are currently sending via documentInfoChanged is immediately available in the guest frame.

In the future, we could easily add a capability to PortFinder#discover to send a frame identifier.

Yes. That's definitely one possibility.

robertknight · 2022-01-27T13:46:46Z

Regarding the VitalSource comments above, this was addressed by #4145.

esanzgar

It looks good. I left a couple of comments.

esanzgar · 2022-02-01T15:47:48Z

src/sidebar/services/frame-sync.js

@@ -36,6 +36,30 @@ export function formatAnnot({ $tag, target, uri }) {
  };
 }

+/**
+ * Return the frame in `frames` which best matches `ann`.


Suggested change

* Return the frame in `frames` which best matches `ann`.

* Return the frame which best matches the annotation.

esanzgar · 2022-02-01T15:49:54Z

src/sidebar/services/frame-sync.js

+  // Choose the frame whose URL exactly matches this annotation. If there is
+  // none, we'll use the main frame.


I think these couple of lines should be included in the functions's documentation.

esanzgar · 2022-02-01T16:40:58Z

src/sidebar/services/frame-sync.js

+    let frameIdentifier = /** @type {string|null} */ (
+      `temp-${this._nextGuestId}`
+    );
+    this._guestRPC.set(frameIdentifier, guestRPC);


In the future, we could easily add a capability to PortFinder#discover to send a frame identifier.

Alternatively, if gathering the information for the documentInfoChanged event is slow, the frame identifier could be send using a new event (decoupled from documentInfoChanged). The frame identifier that are currently sending via documentInfoChanged is immediately available in the guest frame.

When the sidebar is connected to multiple guest frames it will send all incoming annotations to all frames. The result is typically that the annotation will anchor in one frame and orphan in the others. Depending on what order this happens in, the annotation will non-deterministically show up as an Annotation or Orphan in the sidebar. In order to determine which frames an annotation should be sent to in all cases, we'd either need the backend to return information about which search URIs an annotation matches or make a separate search request for each frame and record the associated frame with the results. This will require some significant refactoring of the annotation search service. As an interim step, make `FrameSyncService` send annotations only to a single frame based on matching URL, with a fallback to sending to the main frame if there is no exact match. This will work as expected for most pages, and is at least deterministic when it does fail. When we have a solution for being able to match annotations to frames more generally, we can adapt this code to use it. This is a partial solution to #3992.

robertknight mentioned this pull request Jan 21, 2022

Revise how guest notifies host and sidebar when it is unloaded #4129

Merged

robertknight force-pushed the disconnect-guest-when-unloaded branch from e34cffb to 0247587 Compare January 26, 2022 14:47

robertknight force-pushed the send-annotation-to-single-frame branch from 6ded1d1 to 752da85 Compare January 26, 2022 14:48

robertknight commented Jan 27, 2022

View reviewed changes

robertknight marked this pull request as ready for review January 27, 2022 13:45

robertknight force-pushed the disconnect-guest-when-unloaded branch from 0247587 to 06df56a Compare January 31, 2022 12:16

esanzgar linked an issue Feb 1, 2022 that may be closed by this pull request

Inconsistent re-anchoring in multi-guest scenarios #3992

Closed

robertknight force-pushed the send-annotation-to-single-frame branch from 254ee79 to 9765327 Compare February 1, 2022 16:04

esanzgar approved these changes Feb 2, 2022

View reviewed changes

Base automatically changed from disconnect-guest-when-unloaded to master February 2, 2022 14:10

robertknight added 4 commits February 2, 2022 14:11

Add test to check that annotations are only sent to expected frame

330ba22

Explain why new guests must be added to _guestRPC map immediately

7e02c08

Revise comments for frameForAnnotation

989be8c

robertknight force-pushed the send-annotation-to-single-frame branch from 9765327 to 989be8c Compare February 2, 2022 14:18

robertknight merged commit 4c9bdda into master Feb 2, 2022

robertknight deleted the send-annotation-to-single-frame branch February 2, 2022 14:26

This was referenced Feb 7, 2022

Readium demo not working reliably #4153

Open

Send annotations to correct frame when annotation URL does not match frame URL #4191

Open

Send annotation to first frame when there is no URI match or main frame #4202

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only send annotations to matching frame #4131

Only send annotations to matching frame #4131

robertknight commented Jan 21, 2022 •

edited

Loading

codecov bot commented Jan 21, 2022 •

edited

Loading

robertknight commented Jan 21, 2022

robertknight commented Jan 24, 2022

robertknight Jan 27, 2022

esanzgar Feb 1, 2022

robertknight Feb 2, 2022

robertknight commented Jan 27, 2022

esanzgar left a comment

esanzgar Feb 1, 2022

esanzgar Feb 1, 2022

esanzgar Feb 1, 2022

	* Return the frame in `frames` which best matches `ann`.
	* Return the frame which best matches the annotation.

		// Choose the frame whose URL exactly matches this annotation. If there is
		// none, we'll use the main frame.

Only send annotations to matching frame #4131

Only send annotations to matching frame #4131

Conversation

robertknight commented Jan 21, 2022 • edited Loading

codecov bot commented Jan 21, 2022 • edited Loading

Codecov Report

robertknight commented Jan 21, 2022

robertknight commented Jan 24, 2022

robertknight Jan 27, 2022

Choose a reason for hiding this comment

esanzgar Feb 1, 2022

Choose a reason for hiding this comment

robertknight Feb 2, 2022

Choose a reason for hiding this comment

robertknight commented Jan 27, 2022

esanzgar left a comment

Choose a reason for hiding this comment

esanzgar Feb 1, 2022

Choose a reason for hiding this comment

esanzgar Feb 1, 2022

Choose a reason for hiding this comment

esanzgar Feb 1, 2022

Choose a reason for hiding this comment

robertknight commented Jan 21, 2022 •

edited

Loading

codecov bot commented Jan 21, 2022 •

edited

Loading