Make highlight visibility state work in a multi-guest world #3830

robertknight · 2021-10-14T08:58:55Z

Make the client's highlight visibility state work in a sane way in
scenarios where there are multiple guests, or the main guest frame is not the
host frame. Previously there were several problems in these scenarios:

It was unclear which component of the application "owned" the
highlight visibility state. The state in the sidebar could be changed
as a result of the highlightsVisibleChanged event emitted by a
Guest, as well as the user toggling highlight controls in the
sidebar. This confusion created scenarios where the state could get out of sync with the sidebar in some guests.
In the sidebar's open and close methods it directly set the
highlight visibility in the main guest if the showHighlights
setting was set to whenSidebarOpen. This meant that it didn't work
for guests that were not in the main frame.
Guest frames could be configured with different showHighlights
settings. In this case it was unclear what should happen.

This commit resolves this by making the Sidebar class in the host
frame the central owner of this state. It handles configuring the
initial state based on the showHighlights configuration setting, and
reflecting this state to the sidebar application which in turn reflects
it to guest frames. The initial visibility of highlights in a guest
frame is synchronized with this state when the guest frame connects to
the sidebar. This state is updated by the Sidebar class when:

The user toggles the highlight visibility control in the sidebar
A new highlight or annotation is created in a guest frame
The sidebar opens and closes, if the showHighlights configuration
was set to whenSidebarOpen

Additionally the inconsistency of setHighlightsVisible vs
setVisibleHighlights in identifier and event names has been resolved by
using setHighlightsVisible/highlightsVisible everywhere.

Part of #3798

Testing:

Go to http://localhost:3000/document/parent-frame and make annotations in the parent and child frame.
Toggle highlight visibility in the sidebar. The visibility should be updated in both frames
Add the following snippet to dev-server/documents/html/parent-frame.mustache and verify that visibility of highlights correctly changes when opening and closing the sidebar:

<script type="application/json" class="js-hypothesis-config">
  { "showHighlights": "whenSidebarOpen" }
</script>

Disable highlights and then make a highlight or annotation in either the child or parent frames. This should cause highlights to become visible and the toolbar controls should update to reflect this.

~~A related scenario that doesn't work yet is that making a highlight or annotation in the child frame when highlights are hidden does not show highlights.~~ (Edit: This is now resolved. See second commit.)

codecov · 2021-10-14T09:04:37Z

Codecov Report

Merging #3830 (e63c074) into master (0b757c6) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head e63c074 differs from pull request most recent head 7f2c0b8. Consider uploading reports for the commit 7f2c0b8 to get more accurate results

@@            Coverage Diff             @@
##           master    #3830      +/-   ##
==========================================
+ Coverage   98.96%   98.98%   +0.02%     
==========================================
  Files         211      211              
  Lines        7903     7906       +3     
  Branches     1867     1868       +1     
==========================================
+ Hits         7821     7826       +5     
+ Misses         82       80       -2

Impacted Files	Coverage Δ
src/annotator/config/index.js	`100.00% <ø> (ø)`
src/sidebar/store/modules/viewer.js	`100.00% <ø> (ø)`
src/annotator/guest.js	`99.15% <100.00%> (+0.41%)`	⬆️
src/annotator/sidebar.js	`98.23% <100.00%> (+0.50%)`	⬆️
src/sidebar/services/frame-sync.js	`100.00% <100.00%> (ø)`
src/shared/bridge.js	`100.00% <0.00%> (ø)`
src/annotator/features.js	`100.00% <0.00%> (ø)`
src/annotator/annotation-sync.js	`100.00% <0.00%> (ø)`
src/sidebar/components/TopBar.js	`100.00% <0.00%> (ø)`
src/sidebar/services/features.js	`100.00% <0.00%> (ø)`
... and 6 more

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 0b757c6...7f2c0b8. Read the comment docs.

robertknight · 2021-10-14T10:50:46Z

src/sidebar/services/frame-sync.js

+        // Ensure that the highlight for the newly-created annotation is visible.
+        // Currently we only support a single, shared visibility state for all highlights
+        // in all frames, so this will make all existing highlights visible too.
+        this._hostRPC.call('showHighlights');


To recap the sequence of events here when creating a new highlight while the "Show highlights" control is disabled:

The guest will create a new annotation and send it to the sidebar via a beforeCreateAnnotation request

The sidebar sends a showHighlights request to the host frame

The host frame updates the "Show highlights" control and then sends a setHighlightsVisible request back to the sidebar app

The sidebar app will relay the setHighlightsVisible request to all connected guests, so that the highlight visibility is the same in all of them

The guests handle the setHighlightsVisible request and show or hide highlights in their respective frames

This might seem rather roundabout, but steps 3-5 are the same as when toggling the "Show highlights" control manually, so we avoid having multiple code paths to handle keeping the state in sync across all frames.

In future if we have a direct guest <-> host channel for each guest, we might be able to eliminate the relaying of the setHighlightsVisible message through the sidebar app. CC @esanzgar.

I have been thinking about guest-host channel too, while working on #3838 and realising about the relaying of certain messages host -> sidebar -> guest.

I would support the creation of guest-host channel.

esanzgar · 2021-10-18T10:03:08Z

src/annotator/sidebar.js

@@ -239,6 +244,9 @@ export default class Sidebar {
    sidebarTrigger(document.body, () => this.open());
    features.init(this._sidebarRPC);

+    this._sidebarRPC.on('showHighlights', () =>


showHighlights is a new event type that is sent from the sidebar to the host frame.

Yes. These lines reported errors as expected after a rebase and I've updated the types.

esanzgar · 2021-10-18T10:07:47Z

src/annotator/sidebar.js

+  setHighlightsVisible(visible) {
+    this.toolbar.highlightsVisible = visible;
+
+    // Notify sidebar app of change which will in turn reflect state to guest frames.


Suggested change

// Notify sidebar app of change which will in turn reflect state to guest frames.

// Notify sidebar frame of change which will in turn reflect state to guest frames.

esanzgar · 2021-10-18T10:57:57Z

src/annotator/test/sidebar-test.js

+
+      const call = fakeBridge.call
+        .getCalls()
+        .find(args => args[0] === 'setHighlightsVisible' && args[1] === true);


fakeBridge.call is not invoked at all, with either true or false argument:

Suggested change

.find(args => args[0] === 'setHighlightsVisible' && args[1] === true);

.find(args => args[0] === 'setHighlightsVisible');

fakeBridge.call is not invoked at all, with either true or false argument:

It is called with the sidebarOpened message. I discovered there is a neverCalledWith assertion which can simplify this check though.

esanzgar · 2021-10-18T11:13:00Z

src/sidebar/services/frame-sync.js

@@ -67,6 +67,9 @@ export class FrameSyncService {
    // Set of tags of annotations that are currently loaded into the frame
    const inFrame = new Set();

+    /** Whether highlights are visible in guest frames. */
+    this._highlightsVisible = false;


A single state for all the guest frames.

esanzgar · 2021-10-18T11:28:32Z

src/sidebar/store/modules/test/viewer-test.js

-  describe('#setShowHighlights', () => {
-    it('sets a flag indicating that highlights are visible', () => {
-      store.setShowHighlights(true);
-      assert.isTrue(store.getState().viewer.visibleHighlights);
-    });
-
-    it('sets a flag indicating that highlights are not visible', () => {
-      store.setShowHighlights(false);
-      assert.isFalse(store.getState().viewer.visibleHighlights);
-    });
-  });
-


I am a little confuse by the previous uses of the store. Was this old code or has the reliance of the store been disable in the PR?

This was old code that was no longer used.

esanzgar

I have tested the PR and it works as explained.

There was an action to set this state but it was never read anywhere.

Make the client's highlight visibility state work in a sane way in scenarios where there are multiple guests, or the main guest frame is not the host frame. Previously there were several problems in these scenarios: - It was unclear which component of the application "owned" the highlight visibility state. The state in the sidebar could be changed as a result of the `highlightsVisibleChanged` event emitted by a Guest, as well as the user toggling highlight controls in the sidebar. - In the sidebar's `open` and `close` methods it directly set the highlight visibility in the main guest if the `showHighlights` setting was set to `whenSidebarOpen`. This meant that it didn't work for guests that were not in the main frame. - Guest frames could be configured with different `showHighlights` settings. In this case it was unclear what should happen. This commit resolves this by making the `Sidebar` class in the host frame the central owner of this state. It handles configuring the initial state based on the `showHighlights` configuration setting, and reflecting this state to the sidebar application which in turn reflects it to guest frames. The initial visibility of highlights in a guest frame is synchronized with this state when the guest frame connects to the sidebar. This state is updated by the `Sidebar` class when: - The user toggles the highlight visibility control in the sidebar - A new highlight or annotation is created in a guest frame - The sidebar opens and closes, if the `showHighlights` configuration was set to `whenSidebarOpen` Additionally the inconsistency of `setHighlightsVisible` vs `setVisibleHighlights` in identifier and event names has been resolved by using `setHighlightsVisible`/`highlightsVisible` everywhere. Part of #3798

Change the logic for making highlights visible when creating a new annotation, so that it works if the annotation was created in a non-host frame. This is done by moving the logic from the `beforeAnnotationCreated` handler in the Sidebar class, which was only called for annotations in the host frame, to the `beforeCreateAnnotation` handler in FrameSyncService, which is called for all frames. The sidebar app will then send a request to show highlights to the host frame, which will update the sidebar's controls and then relay the request to guest frames.

robertknight mentioned this pull request Oct 14, 2021

Issues with guest frame support that impact VitalSource #3798

Closed

5 tasks

robertknight commented Oct 14, 2021

View reviewed changes

esanzgar self-requested a review October 14, 2021 14:51

robertknight mentioned this pull request Oct 18, 2021

Strengthen the types for Bridge events #3838

Merged

esanzgar reviewed Oct 18, 2021

View reviewed changes

esanzgar approved these changes Oct 18, 2021

View reviewed changes

robertknight force-pushed the redo-highlight-visibility branch from 94fb864 to 47e35ef Compare October 19, 2021 13:03

robertknight added 4 commits October 19, 2021 15:05

Remove unused visibleHighlights state in store

425b6e7

There was an action to set this state but it was never read anywhere.

Simplify a test assertion using neverCalledWith

7f2c0b8

robertknight force-pushed the redo-highlight-visibility branch from e63c074 to 7f2c0b8 Compare October 19, 2021 14:25

robertknight merged commit 5707862 into master Oct 19, 2021

robertknight deleted the redo-highlight-visibility branch October 19, 2021 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make highlight visibility state work in a multi-guest world #3830

Make highlight visibility state work in a multi-guest world #3830

robertknight commented Oct 14, 2021 •

edited

Loading

codecov bot commented Oct 14, 2021 •

edited

Loading

robertknight Oct 14, 2021 •

edited

Loading

esanzgar Oct 18, 2021

esanzgar Oct 18, 2021 •

edited

Loading

robertknight Oct 19, 2021

esanzgar Oct 18, 2021

esanzgar Oct 18, 2021 •

edited

Loading

robertknight Oct 19, 2021

esanzgar Oct 18, 2021

esanzgar Oct 18, 2021

robertknight Oct 19, 2021

esanzgar left a comment

	// Notify sidebar app of change which will in turn reflect state to guest frames.
	// Notify sidebar frame of change which will in turn reflect state to guest frames.

	.find(args => args[0] === 'setHighlightsVisible' && args[1] === true);
	.find(args => args[0] === 'setHighlightsVisible');

Make highlight visibility state work in a multi-guest world #3830

Make highlight visibility state work in a multi-guest world #3830

Conversation

robertknight commented Oct 14, 2021 • edited Loading

codecov bot commented Oct 14, 2021 • edited Loading

Codecov Report

robertknight Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

esanzgar Oct 18, 2021

Choose a reason for hiding this comment

esanzgar Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

robertknight Oct 19, 2021

Choose a reason for hiding this comment

esanzgar Oct 18, 2021

Choose a reason for hiding this comment

esanzgar Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

robertknight Oct 19, 2021

Choose a reason for hiding this comment

esanzgar Oct 18, 2021

Choose a reason for hiding this comment

esanzgar Oct 18, 2021

Choose a reason for hiding this comment

robertknight Oct 19, 2021

Choose a reason for hiding this comment

esanzgar left a comment

Choose a reason for hiding this comment

robertknight commented Oct 14, 2021 •

edited

Loading

codecov bot commented Oct 14, 2021 •

edited

Loading

robertknight Oct 14, 2021 •

edited

Loading

esanzgar Oct 18, 2021 •

edited

Loading

esanzgar Oct 18, 2021 •

edited

Loading