Make highlight visibility setting work in a multi-guest world

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

src/annotator/config/index.js

@@ -29,7 +29,7 @@ import { urlFromLinkTag } from './url-from-link-tag';
  */
 function configurationKeys(appContext) {
   const contexts = {
-    annotator: ['clientUrl', 'showHighlights', 'subFrameIdentifier'],
+    annotator: ['clientUrl', 'subFrameIdentifier'],
     sidebar: [
       'appType',
       'annotations',

src/annotator/config/test/index-test.js

@@ -216,7 +216,7 @@ describe('annotator/config/index', () => {
     [
       {
         app: 'annotator',
-        expectedKeys: ['clientUrl', 'showHighlights', 'subFrameIdentifier'],
+        expectedKeys: ['clientUrl', 'subFrameIdentifier'],
       },
       {
         app: 'sidebar',
@@ -252,7 +252,7 @@ describe('annotator/config/index', () => {
         ],
       },
     ].forEach(test => {
-      it(`ignore values not belonging to "${test.app}" context`, () => {
+      it(`ignores values not belonging to "${test.app}" context`, () => {
         const config = getConfig(test.app, 'WINDOW');
         assert.deepEqual(Object.keys(config), test.expectedKeys);
       });

src/annotator/guest.js

@@ -122,7 +122,7 @@ export default class Guest {
   constructor(element, eventBus, config = {}, hostFrame = window) {
     this.element = element;
     this._emitter = eventBus.createEmitter();
-    this._visibleHighlights = false;
+    this._highlightsVisible = false;
     this._isAdderVisible = false;
 
     this._adder = new Adder(this.element, {
@@ -131,7 +131,6 @@ export default class Guest {
         /** @type {Selection} */ (document.getSelection()).removeAllRanges();
       },
       onHighlight: async () => {
-        this.setVisibleHighlights(true);
         await this.createAnnotation({ highlight: true });
         /** @type {Selection} */ (document.getSelection()).removeAllRanges();
       },
@@ -165,9 +164,6 @@ export default class Guest {
 
     // Set up listeners for messages coming from the sidebar to this guest.
     this._bridge = new Bridge();
-    this._bridge.onConnect(() => {
-      this.setVisibleHighlights(config.showHighlights === 'always');
-    });
     this._connectSidebarEvents();
 
     // Set up listeners for when the sidebar asks us to add or remove annotations
@@ -226,7 +222,7 @@ export default class Guest {
     this._listeners.add(this.element, 'mouseup', event => {
       const { target, metaKey, ctrlKey } = /** @type {MouseEvent} */ (event);
       const annotations = annotationsAt(/** @type {Element} */ (target));
-      if (annotations.length && this._visibleHighlights) {
+      if (annotations.length && this._highlightsVisible) {
         const toggle = metaKey || ctrlKey;
         this.selectAnnotations(annotations, toggle);
       }
@@ -244,13 +240,13 @@ export default class Guest {
 
     this._listeners.add(this.element, 'mouseover', ({ target }) => {
       const annotations = annotationsAt(/** @type {Element} */ (target));
-      if (annotations.length && this._visibleHighlights) {
+      if (annotations.length && this._highlightsVisible) {
         this._focusAnnotations(annotations);
       }
     });
 
     this._listeners.add(this.element, 'mouseout', () => {
-      if (this._visibleHighlights) {
+      if (this._highlightsVisible) {
         this._focusAnnotations([]);
       }
     });
@@ -354,8 +350,8 @@ export default class Guest {
     });
 
     // Handler for controls on the sidebar
-    this._bridge.on('setVisibleHighlights', showHighlights => {
-      this.setVisibleHighlights(showHighlights);
+    this._bridge.on('setHighlightsVisible', showHighlights => {
+      this.setHighlightsVisible(showHighlights);
     });
   }
 
@@ -661,12 +657,11 @@ export default class Guest {
   /**
    * Set whether highlights are visible in the document or not.
    *
-   * @param {boolean} shouldShowHighlights
+   * @param {boolean} visible
    */
-  setVisibleHighlights(shouldShowHighlights) {
-    setHighlightsVisible(this.element, shouldShowHighlights);
-    this._visibleHighlights = shouldShowHighlights;
-    this._emitter.publish('highlightsVisibleChanged', shouldShowHighlights);
+  setHighlightsVisible(visible) {
+    setHighlightsVisible(this.element, visible);
+    this._highlightsVisible = visible;
   }
 
   /**

src/annotator/sidebar.js

@@ -114,6 +114,7 @@ export default class Sidebar {
       // When a new non-highlight annotation is created, focus
       // the sidebar so that the text editor can be focused as
       // soon as the annotation card appears
+      this.setHighlightsVisible(true);
       if (!annotation.$highlight) {
         /** @type {Window} */ (this.iframe.contentWindow).focus();
       }
@@ -124,7 +125,7 @@ export default class Sidebar {
     this.toolbar = new ToolbarController(toolbarContainer, {
       createAnnotation: () => guest.createAnnotation(),
       setSidebarOpen: open => (open ? this.open() : this.close()),
-      setHighlightsVisible: show => this.setAllVisibleHighlights(show),
+      setHighlightsVisible: show => this.setHighlightsVisible(show),
     });
 
     if (config.theme === 'clean') {
@@ -133,9 +134,6 @@ export default class Sidebar {
       this.toolbar.useMinimalControls = false;
     }
 
-    this._emitter.subscribe('highlightsVisibleChanged', visible => {
-      this.toolbar.highlightsVisible = visible;
-    });
     this._emitter.subscribe('hasSelectionChanged', hasSelection => {
       this.toolbar.newAnnotationType = hasSelection ? 'annotation' : 'note';
     });
@@ -200,6 +198,12 @@ export default class Sidebar {
         this.iframeContainer.style.display = '';
       }
 
+      // Set initial highlight visibility. We do this only once the sidebar app
+      // is ready because `setHighlightsVisible` needs to reflect this state to
+      // the sidebar app.
+      const showHighlights = config.showHighlights === 'always';
+      this.setHighlightsVisible(showHighlights);
+
       if (
         config.openSidebar ||
         config.annotations ||
@@ -451,7 +455,7 @@ export default class Sidebar {
     this.toolbar.sidebarOpen = true;
 
     if (this.options.showHighlights === 'whenSidebarOpen') {
-      this.guest.setVisibleHighlights(true);
+      this.setHighlightsVisible(true);
     }
 
     this._notifyOfLayoutChange(true);
@@ -466,19 +470,22 @@ export default class Sidebar {
     this.toolbar.sidebarOpen = false;
 
     if (this.options.showHighlights === 'whenSidebarOpen') {
-      this.guest.setVisibleHighlights(false);
+      this.setHighlightsVisible(false);
     }
 
     this._notifyOfLayoutChange(false);
   }
 
   /**
-   * Hide or show highlights associated with annotations in the document.
+   * Set whether highlights are visible in guest frames.
    *
-   * @param {boolean} shouldShowHighlights
+   * @param {boolean} visible
    */
-  setAllVisibleHighlights(shouldShowHighlights) {
-    this._sidebarRPC.call('setVisibleHighlights', shouldShowHighlights);
+  setHighlightsVisible(visible) {
+    this.toolbar.highlightsVisible = visible;
+
+    // Notify sidebar app of change which will in turn reflect state to guest frames.
+    this._sidebarRPC.call('setHighlightsVisible', visible);
   }
 
   /**

src/annotator/test/guest-test.js

@@ -87,7 +87,6 @@ describe('Guest', () => {
       createChannel: sinon.stub(),
       destroy: sinon.stub(),
       on: sinon.stub(),
-      onConnect: sinon.stub(),
     };
     FakeBridge = sinon.stub().returns(fakeBridge);
 
@@ -407,18 +406,18 @@ describe('Guest', () => {
       });
     });
 
-    describe('on "setVisibleHighlights" event', () => {
+    describe('on "setHighlightsVisible" event', () => {
       it('sets visibility of highlights in document', () => {
         const guest = createGuest();
 
-        emitGuestEvent('setVisibleHighlights', true);
+        emitGuestEvent('setHighlightsVisible', true);
         assert.calledWith(
           highlighter.setHighlightsVisible,
           guest.element,
           true
         );
 
-        emitGuestEvent('setVisibleHighlights', false);
+        emitGuestEvent('setHighlightsVisible', false);
         assert.calledWith(
           highlighter.setHighlightsVisible,
           guest.element,
@@ -437,7 +436,7 @@ describe('Guest', () => {
     beforeEach(() => {
       fakeSidebarFrame = null;
       guest = createGuest();
-      guest.setVisibleHighlights(true);
+      guest.setHighlightsVisible(true);
       rootElement = guest.element;
 
       // Create a fake highlight as a target for hover and click events.
@@ -520,7 +519,7 @@ describe('Guest', () => {
     });
 
     it('does not focus or select annotations in the sidebar if highlights are hidden', () => {
-      guest.setVisibleHighlights(false);
+      guest.setHighlightsVisible(false);
 
       fakeHighlight.dispatchEvent(new Event('mouseover', { bubbles: true }));
       fakeHighlight.dispatchEvent(new Event('mouseup', { bubbles: true }));

src/annotator/test/sidebar-test.js

@@ -92,7 +92,7 @@ describe('Sidebar', () => {
         this.contentContainer = sinon.stub().returns(document.body);
         this.createAnnotation = sinon.stub();
         this.fitSideBySide = sinon.stub();
-        this.setVisibleHighlights = sinon.stub();
+        this.setHighlightsVisible = sinon.stub();
       }
     }
     fakeGuest = new FakeGuest();
@@ -278,14 +278,14 @@ describe('Sidebar', () => {
 
     it('shows or hides highlights when toolbar button is clicked', () => {
       const sidebar = createSidebar();
-      sinon.stub(sidebar, 'setAllVisibleHighlights');
+      sinon.stub(sidebar, 'setHighlightsVisible');
 
       FakeToolbarController.args[0][1].setHighlightsVisible(true);
-      assert.calledWith(sidebar.setAllVisibleHighlights, true);
-      sidebar.setAllVisibleHighlights.resetHistory();
+      assert.calledWith(sidebar.setHighlightsVisible, true);
+      sidebar.setHighlightsVisible.resetHistory();
 
       FakeToolbarController.args[0][1].setHighlightsVisible(false);
-      assert.calledWith(sidebar.setAllVisibleHighlights, false);
+      assert.calledWith(sidebar.setHighlightsVisible, false);
     });
 
     it('creates an annotation when toolbar button is clicked', () => {
@@ -596,13 +596,17 @@ describe('Sidebar', () => {
     it('shows highlights if "showHighlights" is set to "whenSidebarOpen"', () => {
       const sidebar = createSidebar({ showHighlights: 'whenSidebarOpen' });
       sidebar.open();
-      assert.calledWith(sidebar.guest.setVisibleHighlights, true);
+      assert.calledWith(fakeBridge.call, 'setHighlightsVisible', true);
     });
 
     it('does not show highlights otherwise', () => {
       const sidebar = createSidebar({ showHighlights: 'never' });
       sidebar.open();
-      assert.notCalled(sidebar.guest.setVisibleHighlights);
+
+      const call = fakeBridge.call
+        .getCalls()
+        .find(args => args[0] === 'setHighlightsVisible' && args[1] === true);
+      assert.isUndefined(call);
     });
 
     it('updates the `sidebarOpen` property of the toolbar', () => {
@@ -619,7 +623,7 @@ describe('Sidebar', () => {
       sidebar.open();
       sidebar.close();
 
-      assert.calledWith(sidebar.guest.setVisibleHighlights, false);
+      assert.calledWith(fakeBridge.call, 'setHighlightsVisible', false);
     });
 
     it('updates the `sidebarOpen` property of the toolbar', () => {
@@ -632,12 +636,25 @@ describe('Sidebar', () => {
     });
   });
 
-  describe('#setAllVisibleHighlights', () =>
+  describe('#setHighlightsVisible', () => {
     it('requests sidebar to set highlight visibility in guest frames', () => {
       const sidebar = createSidebar();
-      sidebar.setAllVisibleHighlights(true);
-      assert.calledWith(fakeBridge.call, 'setVisibleHighlights', true);
-    }));
+      sidebar.setHighlightsVisible(true);
+      assert.calledWith(fakeBridge.call, 'setHighlightsVisible', true);
+
+      sidebar.setHighlightsVisible(false);
+      assert.calledWith(fakeBridge.call, 'setHighlightsVisible', false);
+    });
+
+    it('toggles "Show highlights" control in toolbar', () => {
+      const sidebar = createSidebar();
+      sidebar.setHighlightsVisible(true);
+      assert.isTrue(fakeToolbar.highlightsVisible);
+
+      sidebar.setHighlightsVisible(false);
+      assert.isFalse(fakeToolbar.highlightsVisible);
+    });
+  });
 
   it('hides toolbar controls when using the "clean" theme', () => {
     createSidebar({ theme: 'clean' });

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;
+
     /**
      * Watch for changes to the set of annotations displayed in the sidebar and
      * notify connected guests about new/updated/deleted annotations.
@@ -212,6 +215,9 @@ export class FrameSyncService {
      * @param {PortRPC} channel
      */
     const addFrame = channel => {
+      // Synchronize highlight visibility in this guest with the sidebar's controls.
+      channel.call('setHighlightsVisible', this._highlightsVisible);
+
       channel.call('getDocumentInfo', (err, info) => {
         if (err) {
           channel.destroy();
@@ -265,8 +271,9 @@ export class FrameSyncService {
 
     // When user toggles the highlight visibility control in the sidebar container,
     // update the visibility in all the guest frames.
-    this._hostRPC.on('setVisibleHighlights', state => {
-      this._guestRPC.call('setVisibleHighlights', state);
+    this._hostRPC.on('setHighlightsVisible', visible => {
+      this._highlightsVisible = visible;
+      this._guestRPC.call('setHighlightsVisible', visible);
     });
 
     // Create channel for sidebar <-> host communication and send port to host.