Toggle annotation/note button in multi-frame scenarios

In order to toggle correctly the `Toolbar` button between `New page
note` and `New annotation` we have created a new `guest-host` channel of
communication. Previous code failed to toggle the button because it used
an event emitter, hence assuming that the `Guest` was in the same frame
as the host.

- The `Sidebar` keeps track of the iframe that has currently text
  selected. It uses the `subFrameIdentifier`. If there is no
  `subFrameIdentifier` (currently the top `Guest` frame), it sets the
  identifier to 'main', referring to the frame with the main annotatable
  content. This will change in a follow up PR that will add more logic
  into which frame is tagged as 'main'.

- Only one iframe at a time can have selected text.

- The event emitter `hasSelectionChanged` has been replaced by three RPC
  methods: 'textSelectedAt`, 'textDeselectedAt', 'deselecTextExcept'

- `deselecTextExcept` is used to clear text selections in other frames,
  except from the iframe that has the latest selection. This is not the
  neatest solution, but for now it works.

On a follow up PR I will address the removal of the shadow left from
previous a selection.

Closes https://github.com/hypothesis/product-backlog/issues/1236

src/annotator/guest.js

@@ -26,6 +26,8 @@ import { normalizeURI } from './util/url';
  * @typedef {import('../types/annotator').Destroyable} Destroyable
  * @typedef {import('../types/annotator').SidebarLayout} SidebarLayout
  * @typedef {import('../types/api').Target} Target
+ * @typedef {import('../types/bridge-events').HostToGuestEvent} HostToGuestEvent
+ * @typedef {import('../types/bridge-events').GuestToHostEvent} GuestToHostEvent
  * @typedef {import('../types/bridge-events').GuestToSidebarEvent} GuestToSidebarEvent
  * @typedef {import('../types/bridge-events').SidebarToGuestEvent} SidebarToGuestEvent
  * @typedef {import('./util/emitter').EventBus} EventBus
@@ -102,8 +104,9 @@ function resolveAnchor(anchor) {
  * loads Hypothesis (not all frames will be annotation-enabled). In one frame,
  * usually the top-level one, there will also be an instance of the `Sidebar`
  * class that shows the sidebar app and surrounding UI. The `Guest` instance in
- * each frame connects to the sidebar as part of its initialization, when
- * {@link _connectSidebarEvents} is called.
+ * each frame connects to the sidebar frame as part of its initialization, when
+ * {@link _connectSidebarEvents} is called. The `Guest` is also connected to the
+ * host frame using the {@link _connectHostEvents} method.
  *
  * The anchoring implementation defaults to a generic one for HTML documents and
  * can be overridden to handle different document types.
@@ -164,14 +167,23 @@ export default class Guest {
     this.anchors = [];
 
     // Set the frame identifier if it's available.
-    // The "top" guest instance will have this as null since it's in a top frame not a sub frame
-    /** @type {string|null} */
-    this._frameIdentifier = config.subFrameIdentifier || null;
+    // The "top" guest instance will have this as 'main' since it's in a top frame not a sub frame
+    /** @type {string} */
+    this._frameIdentifier = config.subFrameIdentifier ?? 'main';
+
     this._portFinder = new PortFinder({
       hostFrame: this._hostFrame,
       source: 'guest',
     });
 
+    /**
+     * Channel for host-guest communication.
+     *
+     * @type {Bridge<GuestToHostEvent,HostToGuestEvent>}
+     */
+    this._hostRPC = new Bridge();
+    this._connectHostEvents();
+
     /**
      * Channel for guest-sidebar communication.
      *
@@ -300,6 +312,37 @@ export default class Guest {
     }
   }
 
+  async _connectHostEvents() {
+    this._hostRPC.on(
+      'deselectTextExcept',
+      /** @type {string} */ frameIdentifier => {
+        if (this._frameIdentifier === frameIdentifier) {
+          return;
+        }
+
+        this._onClearSelection({ informHostFrame: false });
+        // It would be nice to clear the shadow of the selection calling
+        // `document.getSelection()?.removeAllRanges();` however, because of the
+        // selection observer, that will trigger a call to `_onClearSelection({informHostFrame: true})`
+        // which in turn will remove the selection from the `Guest` that has the selection.
+      }
+    );
+
+    this._hostRPC.on(
+      'createAnnotationAt',
+      /** @type {string} */ frameIdentifier => {
+        if (this._frameIdentifier === frameIdentifier) {
+          this.createAnnotation();
+        }
+      }
+    );
+
+    // Discover and connect to the host frame. All RPC events must be
+    // registered before creating the channel.
+    const hostPort = await this._portFinder.discover('host');
+    this._hostRPC.createChannel(hostPort);
+  }
+
   async _connectSidebarEvents() {
     // Handlers for events sent when user hovers or clicks on an annotation card
     // in the sidebar.
@@ -612,18 +655,20 @@ export default class Guest {
     }
 
     this.selectedRanges = [range];
-    this._emitter.publish('hasSelectionChanged', true);
+    this._hostRPC.call('textSelectedAt', this._frameIdentifier);
 
     this._adder.annotationsForSelection = annotationsForSelection();
     this._isAdderVisible = true;
     this._adder.show(focusRect, isBackwards);
   }
 
-  _onClearSelection() {
+  _onClearSelection({ informHostFrame = true } = {}) {
     this._isAdderVisible = false;
     this._adder.hide();
     this.selectedRanges = [];
-    this._emitter.publish('hasSelectionChanged', false);
+    if (informHostFrame) {
+      this._hostRPC.call('textDeselectedAt', this._frameIdentifier);
+    }
   }
 
   /**

src/annotator/sidebar.js

@@ -15,6 +15,8 @@ import { createShadowRoot } from './util/shadow-root';
 
 /**
  * @typedef {import('./guest').default} Guest
+ * @typedef {import('../types/bridge-events').GuestToHostEvent} GuestToHostEvent
+ * @typedef {import('../types/bridge-events').HostToGuestEvent} HostToGuestEvent
  * @typedef {import('../types/bridge-events').HostToSidebarEvent} HostToSidebarEvent
  * @typedef {import('../types/bridge-events').SidebarToHostEvent} SidebarToHostEvent
  * @typedef {import('../types/annotator').SidebarLayout} SidebarLayout
@@ -67,6 +69,22 @@ export default class Sidebar {
   constructor(element, eventBus, guest, config = {}) {
     this._emitter = eventBus.createEmitter();
 
+    /**
+     * Tracks which `Guest` has a text selection. It uses the frame i«dentifier
+     * which uniquely identifies each `Guest`. If there is no text selected, the
+     * value is set to `null`.
+     *
+     * @type {null|string}
+     */
+    this._selectionAt = null;
+
+    /**
+     * Channel for host-guest communication.
+     *
+     * @type {Bridge<HostToGuestEvent,GuestToHostEvent>}
+     */
+    this._guestRPC = new Bridge();
+
     /**
      * Channel for host-sidebar communication.
      *
@@ -126,7 +144,8 @@ export default class Sidebar {
     // Set up the toolbar on the left edge of the sidebar.
     const toolbarContainer = document.createElement('div');
     this.toolbar = new ToolbarController(toolbarContainer, {
-      createAnnotation: () => guest.createAnnotation(),
+      createAnnotation: () =>
+        this._guestRPC.call('createAnnotationAt', this._selectionAt ?? 'main'),
       setSidebarOpen: open => (open ? this.open() : this.close()),
       setHighlightsVisible: show => this.setHighlightsVisible(show),
     });
@@ -137,10 +156,6 @@ export default class Sidebar {
       this.toolbar.useMinimalControls = false;
     }
 
-    this._emitter.subscribe('hasSelectionChanged', hasSelection => {
-      this.toolbar.newAnnotationType = hasSelection ? 'annotation' : 'note';
-    });
-
     if (this.iframeContainer) {
       // If using our own container frame for the sidebar, add the toolbar to it.
       this.iframeContainer.prepend(toolbarContainer);
@@ -201,6 +216,8 @@ export default class Sidebar {
       }
     });
 
+    this._setupGuestEvents();
+
     // 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.
@@ -214,6 +231,8 @@ export default class Sidebar {
   }
 
   destroy() {
+    this._guestRPC.destroy();
+    this._sidebarRPC.destroy();
     this.bucketBar?.destroy();
     this._listeners.removeAll();
     this._hammerManager?.destroy();
@@ -235,9 +254,34 @@ export default class Sidebar {
    * @param {MessagePort} port
    */
   onFrameConnected(source, port) {
-    if (source === 'sidebar') {
+    switch (source) {
+      case 'guest':
+        this._guestRPC.createChannel(port);
+        break;
+      case 'sidebar':
         this._sidebarRPC.createChannel(port);
+        break;
+    }
+  }
+
+  _setupGuestEvents() {
+    this._guestRPC.on(
+      'textSelectedAt',
+      /** @type {string} */ frameIdentifier => {
+        this._selectionAt = frameIdentifier;
+        this.toolbar.newAnnotationType = 'annotation';
+        this._guestRPC.call('deselectTextExcept', frameIdentifier);
       }
+    );
+
+    this._guestRPC.on(
+      'textDeselectedAt',
+      /** @type {string} */ frameIdentifier => {
+        this._selectionAt = null;
+        this.toolbar.newAnnotationType = 'note';
+        this._guestRPC.call('deselectTextExcept', frameIdentifier);
+      }
+    );
   }
 
   _setupSidebarEvents() {

src/annotator/test/guest-test.js

@@ -628,24 +628,70 @@ describe('Guest', () => {
       assert.notCalled(FakeAdder.instance.show);
     });
 
-    it('emits `hasSelectionChanged` event with argument `true` if selection is non-empty', () => {
+    it('calls "textSelectionAt" RPC method with argument "main" if selection is non-empty', () => {
+      createGuest();
+
+      simulateSelectionWithText();
+
+      assert.calledWith(fakeBridge.call, 'textSelectedAt', 'main');
+    });
+
+    it('calls "textSelectionAt" RPC method with the subFrameIdentifier as argument if selection is non-empty', () => {
+      const subFrameIdentifier = 'other frame';
+      createGuest({ subFrameIdentifier });
+
+      simulateSelectionWithText();
+
+      assert.calledWith(fakeBridge.call, 'textSelectedAt', subFrameIdentifier);
+    });
+
+    it('calls "textDeselectedAt" RPC method with argument "main" if selection is empty', () => {
+      createGuest();
+
+      simulateSelectionWithoutText();
+
+      assert.calledWith(fakeBridge.call, 'textDeselectedAt', 'main');
+    });
+
+    it('calls "textDeselectedAt" RPC method with the subFrameIdentifier as argument if selection is empty', () => {
+      const subFrameIdentifier = 'other frame';
+      createGuest({ subFrameIdentifier });
+
+      simulateSelectionWithoutText();
+
+      assert.calledWith(
+        fakeBridge.call,
+        'textDeselectedAt',
+        subFrameIdentifier
+      );
+    });
+
+    it('unselects text if another iframe has made a selection', () => {
       const guest = createGuest();
-      const callback = sandbox.stub();
-      guest._emitter.subscribe('hasSelectionChanged', callback);
+      guest.selectedRanges = [1];
+      const handler = fakeBridge.on
+        .getCalls()
+        .find(call => call.args[0] === 'deselectTextExcept').args[1];
 
       simulateSelectionWithText();
+      fakeBridge.call.resetHistory();
+      handler('dummy');
 
-      assert.calledWith(callback, true);
+      assert.equal(guest.selectedRanges.length, 0);
+      assert.notCalled(fakeBridge.call);
     });
 
-    it('emits `hasSelectionChanged` event with argument `false` if selection is empty', () => {
+    it("doesn't unselects text if frame identifiers matches", () => {
       const guest = createGuest();
-      const callback = sandbox.stub();
-      guest._emitter.subscribe('hasSelectionChanged', callback);
+      guest.selectedRanges = [1];
+      const handler = fakeBridge.on
+        .getCalls()
+        .find(call => call.args[0] === 'deselectTextExcept').args[1];
 
-      simulateSelectionWithoutText();
+      simulateSelectionWithText();
+      handler('main'); // doesn't unselect the text because it matches the frameIdentifier
 
-      assert.calledWith(callback, false);
+      assert.equal(guest.selectedRanges.length, 1);
     });
   });
 
@@ -759,6 +805,20 @@ describe('Guest', () => {
   });
 
   describe('#createAnnotation', () => {
+    it('creates an annotation if host calls with "createAnnotationAt" RPC method', () => {
+      const guest = createGuest();
+      sinon.stub(guest, 'createAnnotation');
+      const handler = fakeBridge.on
+        .getCalls()
+        .find(call => call.args[0] === 'createAnnotationAt').args[1];
+
+      handler('dummy');
+      assert.notCalled(guest.createAnnotation);
+
+      handler('main');
+      assert.calledOnce(guest.createAnnotation);
+    });
+
     it('adds document metadata to the annotation', async () => {
       const guest = createGuest();
 

src/annotator/test/sidebar-test.js

@@ -82,6 +82,7 @@ describe('Sidebar', () => {
       createChannel: sinon.stub(),
       on: sinon.stub(),
       onConnect: sinon.stub(),
+      destroy: sinon.stub(),
     };
 
     fakeBucketBar = {
@@ -231,20 +232,36 @@ describe('Sidebar', () => {
       assert.calledWith(sidebar.setHighlightsVisible, false);
     });
 
-    it('creates an annotation when toolbar button is clicked', () => {
-      const sidebar = createSidebar();
+    it('creates an annotation in the main content frame when toolbar button is clicked', () => {
+      createSidebar();
+
+      FakeToolbarController.args[0][1].createAnnotation();
+
+      assert.calledWith(fakeBridge.call, 'createAnnotationAt', 'main');
+    });
+
+    it('creates an annotation on another frame toolbar button is clicked', () => {
+      createSidebar({});
+
+      // make a text selection on another frame
+      const handler = fakeBridge.on
+        .getCalls()
+        .find(call => call.args[0] === 'textSelectedAt').args[1];
+      const frameIdentifier = 'other frame';
+      handler(frameIdentifier);
 
       FakeToolbarController.args[0][1].createAnnotation();
 
-      assert.called(sidebar.guest.createAnnotation);
+      assert.calledWith(fakeBridge.call, 'createAnnotationAt', frameIdentifier);
     });
 
     it('sets create annotation button to "Annotation" when selection becomes non-empty', () => {
       const sidebar = createSidebar();
 
-      // nb. This event is normally published by the Guest, but the sidebar
-      // doesn't care about that.
-      sidebar._emitter.publish('hasSelectionChanged', true);
+      const handler = fakeBridge.on
+        .getCalls()
+        .find(call => call.args[0] === 'textSelectedAt').args[1];
+      handler('dummy');
 
       assert.equal(sidebar.toolbar.newAnnotationType, 'annotation');
     });
@@ -252,9 +269,10 @@ describe('Sidebar', () => {
     it('sets create annotation button to "Page Note" when selection becomes empty', () => {
       const sidebar = createSidebar();
 
-      // nb. This event is normally published by the Guest, but the sidebar
-      // doesn't care about that.
-      sidebar._emitter.publish('hasSelectionChanged', false);
+      const handler = fakeBridge.on
+        .getCalls()
+        .find(call => call.args[0] === 'textDeselectedAt').args[1];
+      handler('dummy');
 
       assert.equal(sidebar.toolbar.newAnnotationType, 'note');
     });
@@ -611,6 +629,16 @@ describe('Sidebar', () => {
     });
   });
 
+  describe('#onFrameConnected', () => {
+    it('creates a channel to communicate to the guests', () => {
+      const sidebar = createSidebar();
+      const { port1 } = new MessageChannel();
+      sidebar.onFrameConnected('guest', port1);
+
+      assert.calledWith(fakeBridge.createChannel, port1);
+    });
+  });
+
   describe('#setHighlightsVisible', () => {
     it('requests sidebar to set highlight visibility in guest frames', () => {
       const sidebar = createSidebar();

src/types/bridge-events.d.ts

@@ -4,23 +4,18 @@
  */
 
 /**
- * Events that the host sends to the sidebar
+ * Events that the guest sends to the host
  */
-export type HostToSidebarEvent =
+export type GuestToHostEvent =
   /**
-   * The host informs the sidebar that a guest frame has been destroyed
+   * The guest informs the host that text has been deselected at a frame with that identifier.
    */
-  | 'frameDestroyed'
+  | 'textDeselectedAt'
 
   /**
-   * Highlights have been toggled on/off.
+   * The guest informs the host that text has been selected at a frame with that identifier.
    */
-  | 'setHighlightsVisible'
-
-  /**
-   * The host informs the sidebar that the sidebar has been opened.
-   */
-  | 'sidebarOpened';
+  | 'textSelectedAt';
 
 /**
  * Events that the guest sends to the sidebar
@@ -61,6 +56,39 @@ export type GuestToSidebarEvent =
    */
   | 'toggleAnnotationSelection';
 
+/**
+ * Events that the host sends to the guest
+ */
+export type HostToGuestEvent =
+  /**
+   * The host request the guest at a frame with that identifier to create an annotation.
+   */
+  | 'createAnnotationAt'
+
+  /**
+   * The host informs the guests that text should be deselected if it doesn't match the current frame identifier.
+   */
+  | 'deselectTextExcept';
+
+/**
+ * Events that the host sends to the sidebar
+ */
+export type HostToSidebarEvent =
+  /**
+   * The host informs the sidebar that a guest frame has been destroyed
+   */
+  | 'frameDestroyed'
+
+  /**
+   * Highlights have been toggled on/off.
+   */
+  | 'setHighlightsVisible'
+
+  /**
+   * The host informs the sidebar that the sidebar has been opened.
+   */
+  | 'sidebarOpened';
+
 /**
  * Events that the sidebar sends to the guest(s)
  */
@@ -166,7 +194,9 @@ export type SidebarToHostEvent =
   | 'signupRequested';
 
 export type BridgeEvent =
+  | HostToGuestEvent
   | HostToSidebarEvent
+  | GuestToHostEvent
   | GuestToSidebarEvent
   | SidebarToGuestEvent
   | SidebarToHostEvent;