Apply suggestions from code review

Co-authored-by: Robert Knight <robertknight@gmail.com>

src/annotator/guest.js

@@ -166,9 +166,9 @@ export default class Guest {
     this.anchors = [];
 
     // Set the frame identifier if it's available.
-    // 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';
+    // 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;
 
     this._portFinder = new PortFinder({
       hostFrame: this._hostFrame,
@@ -314,7 +314,7 @@ export default class Guest {
   async _connectHostEvents() {
     this._hostRPC.on(
       'clearSelectionExceptIn',
-      /** @param {string} frameIdentifier */
+      /** @param {string|null} frameIdentifier */
       frameIdentifier => {
         if (this._frameIdentifier === frameIdentifier) {
           return;
@@ -329,7 +329,7 @@ export default class Guest {
 
     this._hostRPC.on(
       'createAnnotationIn',
-      /** @param {string} frameIdentifier */
+      /** @param {string|null} frameIdentifier */
       frameIdentifier => {
         if (this._frameIdentifier === frameIdentifier) {
           this.createAnnotation();

src/annotator/sidebar.js

@@ -70,9 +70,8 @@ export default class Sidebar {
     this._emitter = eventBus.createEmitter();
 
     /**
-     * Tracks which `Guest` has a text selection. It uses the frame identifier
-     * which uniquely identifies each `Guest`. If there is no text selected, the
-     * value is set to `null`.
+     * Tracks which `Guest` has a text selection. `null` indicates the host frame,
+     * the top-level frame. Other `guest` frames use string identifier.
      *
      * @type {string|null}
      */
@@ -145,10 +144,7 @@ export default class Sidebar {
     const toolbarContainer = document.createElement('div');
     this.toolbar = new ToolbarController(toolbarContainer, {
       createAnnotation: () =>
-        this._guestRPC.call(
-          'createAnnotationIn',
-          this._guestWithSelection ?? 'main'
-        ),
+        this._guestRPC.call('createAnnotationIn', this._guestWithSelection),
       setSidebarOpen: open => (open ? this.open() : this.close()),
       setHighlightsVisible: show => this.setHighlightsVisible(show),
     });
@@ -270,7 +266,7 @@ export default class Sidebar {
   _setupGuestEvents() {
     this._guestRPC.on(
       'textSelectedIn',
-      /** @param {string} frameIdentifier */
+      /** @param {string|null} frameIdentifier */
       frameIdentifier => {
         this._guestWithSelection = frameIdentifier;
         this.toolbar.newAnnotationType = 'annotation';
@@ -280,9 +276,9 @@ export default class Sidebar {
 
     this._guestRPC.on(
       'textUnselectedIn',
-      /** @param {string}  frameIdentifier */
+      /** @param {string|null}  frameIdentifier */
       frameIdentifier => {
-        this._guestWithSelection = null;
+        this._guestWithSelection = null; // default to the `host` frame
         this.toolbar.newAnnotationType = 'note';
         this._guestRPC.call('clearSelectionExceptIn', frameIdentifier);
       }

src/annotator/test/guest-test.js

@@ -628,16 +628,16 @@ describe('Guest', () => {
       assert.notCalled(FakeAdder.instance.show);
     });
 
-    it('calls "textSelectionAt" RPC method with argument "main" if selection is non-empty', () => {
+    it('calls "textSelectedIn" RPC method with argument "null" if selection is non-empty', () => {
       createGuest();
 
       simulateSelectionWithText();
 
-      assert.calledWith(fakeBridge.call, 'textSelectedIn', 'main');
+      assert.calledWith(fakeBridge.call, 'textSelectedIn', null);
     });
 
-    it('calls "textSelectionAt" RPC method with the subFrameIdentifier as argument if selection is non-empty', () => {
-      const subFrameIdentifier = 'other frame';
+    it('calls "textSelectedIn" RPC method with the subFrameIdentifier as argument if selection is non-empty', () => {
+      const subFrameIdentifier = 'subframe identifier';
       createGuest({ subFrameIdentifier });
 
       simulateSelectionWithText();
@@ -645,16 +645,16 @@ describe('Guest', () => {
       assert.calledWith(fakeBridge.call, 'textSelectedIn', subFrameIdentifier);
     });
 
-    it('calls "textUnselectedIn" RPC method with argument "main" if selection is empty', () => {
+    it('calls "textUnselectedIn" RPC method with argument "null" if selection is empty', () => {
       createGuest();
 
       simulateSelectionWithoutText();
 
-      assert.calledWith(fakeBridge.call, 'textUnselectedIn', 'main');
+      assert.calledWith(fakeBridge.call, 'textUnselectedIn', null);
     });
 
     it('calls "textUnselectedIn" RPC method with the subFrameIdentifier as argument if selection is empty', () => {
-      const subFrameIdentifier = 'other frame';
+      const subFrameIdentifier = 'subframe identifier';
       createGuest({ subFrameIdentifier });
 
       simulateSelectionWithoutText();
@@ -675,13 +675,13 @@ describe('Guest', () => {
 
       simulateSelectionWithText();
       fakeBridge.call.resetHistory();
-      handler('dummy');
+      handler('subframe identifier');
 
       assert.equal(guest.selectedRanges.length, 0);
       assert.notCalled(fakeBridge.call);
     });
 
-    it("doesn't deselect text if frame identifiers matches", () => {
+    it("doesn't unselect text if frame identifier matches", () => {
       const guest = createGuest();
       guest.selectedRanges = [1];
       const handler = fakeBridge.on
@@ -689,7 +689,7 @@ describe('Guest', () => {
         .find(call => call.args[0] === 'clearSelectionExceptIn').args[1];
 
       simulateSelectionWithText();
-      handler('main'); // doesn't unselect the text because it matches the frameIdentifier
+      handler(null); // it matches the frameIdentifier in the host frame
 
       assert.equal(guest.selectedRanges.length, 1);
     });
@@ -805,7 +805,7 @@ describe('Guest', () => {
   });
 
   describe('#createAnnotation', () => {
-    it('creates an annotation if host calls with "createAnnotationIn" RPC method', () => {
+    it('creates an annotation if host calls "createAnnotationIn" RPC method', () => {
       const guest = createGuest();
       sinon.stub(guest, 'createAnnotation');
       const handler = fakeBridge.on
@@ -815,7 +815,7 @@ describe('Guest', () => {
       handler('dummy');
       assert.notCalled(guest.createAnnotation);
 
-      handler('main');
+      handler(null);
       assert.calledOnce(guest.createAnnotation);
     });
 

src/annotator/test/sidebar-test.js

@@ -232,22 +232,22 @@ describe('Sidebar', () => {
       assert.calledWith(sidebar.setHighlightsVisible, false);
     });
 
-    it('creates an annotation in the main content frame when toolbar button is clicked', () => {
+    it('creates an annotation in the host frame when toolbar button is clicked', () => {
       createSidebar();
 
       FakeToolbarController.args[0][1].createAnnotation();
 
-      assert.calledWith(fakeBridge.call, 'createAnnotationIn', 'main');
+      assert.calledWith(fakeBridge.call, 'createAnnotationIn', null);
     });
 
-    it('creates an annotation on another frame toolbar button is clicked', () => {
+    it('creates an annotation in frame with selection when toolbar button is clicked', () => {
       createSidebar({});
 
       // Make a text selection in another frame
       const handler = fakeBridge.on
         .getCalls()
         .find(call => call.args[0] === 'textSelectedIn').args[1];
-      const frameIdentifier = 'other frame';
+      const frameIdentifier = 'subframe identifier';
       handler(frameIdentifier);
 
       FakeToolbarController.args[0][1].createAnnotation();
@@ -255,26 +255,38 @@ describe('Sidebar', () => {
       assert.calledWith(fakeBridge.call, 'createAnnotationIn', frameIdentifier);
     });
 
-    it('sets create annotation button to "Annotation" when selection becomes non-empty', () => {
+    it('toggles create annotation button to "Annotation" when selection becomes non-empty', () => {
       const sidebar = createSidebar();
 
+      const frameIdentifier = 'subframe identifier';
       const handler = fakeBridge.on
         .getCalls()
         .find(call => call.args[0] === 'textSelectedIn').args[1];
-      handler('dummy');
+      handler(frameIdentifier);
 
       assert.equal(sidebar.toolbar.newAnnotationType, 'annotation');
+      assert.calledWith(
+        fakeBridge.call,
+        'clearSelectionExceptIn',
+        frameIdentifier
+      );
     });
 
-    it('sets create annotation button to "Page Note" when selection becomes empty', () => {
+    it('toggles create annotation button to "Page Note" when selection becomes empty', () => {
       const sidebar = createSidebar();
 
+      const frameIdentifier = null;
       const handler = fakeBridge.on
         .getCalls()
         .find(call => call.args[0] === 'textUnselectedIn').args[1];
-      handler('dummy');
+      handler(frameIdentifier);
 
       assert.equal(sidebar.toolbar.newAnnotationType, 'note');
+      assert.calledWith(
+        fakeBridge.call,
+        'clearSelectionExceptIn',
+        frameIdentifier
+      );
     });
   });
 

src/types/bridge-events.d.ts

@@ -61,12 +61,12 @@ export type GuestToSidebarEvent =
  */
 export type HostToGuestEvent =
   /**
-   * The host request the guest at a frame with that identifier to create an annotation.
+   * The host requests a guest with a specific frame identifier to create an annotation.
    */
   | 'createAnnotationIn'
 
   /**
-   * The host informs the guests that text should be unselect if it doesn't match the current frame identifier.
+   * The host informs guests that text should be unselected, except in the guest with a given frame identifier
    */
   | 'clearSelectionExceptIn';