From f7f2dc7919080e94078124a7bdf6b03432fe2f24 Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Thu, 14 Oct 2021 09:24:48 +0100 Subject: [PATCH] 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 https://github.com/hypothesis/client/issues/3798 --- src/annotator/config/index.js | 2 +- src/annotator/config/test/index-test.js | 4 +- src/annotator/guest.js | 23 +++++------ src/annotator/sidebar.js | 29 +++++++++----- src/annotator/test/guest-test.js | 10 ++--- src/annotator/test/sidebar-test.js | 41 ++++++++++++++------ src/sidebar/services/frame-sync.js | 11 +++++- src/sidebar/services/test/frame-sync-test.js | 37 +++++++++++++----- 8 files changed, 102 insertions(+), 55 deletions(-) diff --git a/src/annotator/config/index.js b/src/annotator/config/index.js index ae6179e49a8..f1c2917dc29 100644 --- a/src/annotator/config/index.js +++ b/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', diff --git a/src/annotator/config/test/index-test.js b/src/annotator/config/test/index-test.js index 718f8c1c104..800eda31d24 100644 --- a/src/annotator/config/test/index-test.js +++ b/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); }); diff --git a/src/annotator/guest.js b/src/annotator/guest.js index 3fd6d35c5fb..502f4beac2c 100644 --- a/src/annotator/guest.js +++ b/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(); }, @@ -167,7 +166,6 @@ export default class Guest { this._bridge = new Bridge(); this._bridge.onConnect(() => { this._emitter.publish('panelReady'); - this.setVisibleHighlights(config.showHighlights === 'always'); }); this._connectSidebarEvents(); @@ -227,7 +225,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); } @@ -245,13 +243,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([]); } }); @@ -355,8 +353,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); }); } @@ -662,12 +660,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; } /** diff --git a/src/annotator/sidebar.js b/src/annotator/sidebar.js index fa77745b9b8..5eb4fa86c9d 100644 --- a/src/annotator/sidebar.js +++ b/src/annotator/sidebar.js @@ -121,6 +121,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(); } @@ -140,7 +141,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') { @@ -149,9 +150,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'; }); @@ -210,6 +208,14 @@ export default class Sidebar { }); }); + this.ready.then(() => { + // 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); + }); + // 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. @@ -451,7 +457,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 +472,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); } /** diff --git a/src/annotator/test/guest-test.js b/src/annotator/test/guest-test.js index fdfc336a302..45c71ebfd03 100644 --- a/src/annotator/test/guest-test.js +++ b/src/annotator/test/guest-test.js @@ -415,18 +415,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, @@ -445,7 +445,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. @@ -528,7 +528,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 })); diff --git a/src/annotator/test/sidebar-test.js b/src/annotator/test/sidebar-test.js index 47fbec3563b..51eb68cf7ab 100644 --- a/src/annotator/test/sidebar-test.js +++ b/src/annotator/test/sidebar-test.js @@ -73,7 +73,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(); @@ -273,14 +273,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', () => { @@ -599,13 +599,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', () => { @@ -622,7 +626,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', () => { @@ -635,12 +639,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' }); diff --git a/src/sidebar/services/frame-sync.js b/src/sidebar/services/frame-sync.js index a240c3e9abd..e4ec330d262 100644 --- a/src/sidebar/services/frame-sync.js +++ b/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. diff --git a/src/sidebar/services/test/frame-sync-test.js b/src/sidebar/services/test/frame-sync-test.js index 8e4815726c6..84e734f13f0 100644 --- a/src/sidebar/services/test/frame-sync-test.js +++ b/src/sidebar/services/test/frame-sync-test.js @@ -406,14 +406,17 @@ describe('FrameSyncService', () => { context('when a new frame connects', () => { let frameInfo; - const fakeChannel = { - call: function (name, callback) { - callback(null, frameInfo); - }, - destroy: sinon.stub(), - }; + let fakeChannel; beforeEach(() => { + fakeChannel = { + call: sinon.spy((name, callback) => { + if (name === 'getDocumentInfo') { + callback(null, frameInfo); + } + }), + destroy: sinon.stub(), + }; frameSync.connect(); }); @@ -430,13 +433,27 @@ describe('FrameSyncService', () => { }); it('closes the channel and does not add frame to store if getting document info fails', () => { - fakeChannel.call = (name, callback) => callback('Something went wrong'); + fakeChannel.call = (name, callback) => { + if (name === 'getDocumentInfo') { + callback('Something went wrong'); + } + }; guestBridge().emit('connect', fakeChannel); assert.called(fakeChannel.destroy); assert.notCalled(fakeStore.connectFrame); }); + + it("synchronizes highlight visibility in the guest with the sidebar's controls", () => { + hostBridge().emit('setHighlightsVisible', true); + guestBridge().emit('connect', fakeChannel); + assert.calledWith(fakeChannel.call, 'setHighlightsVisible', true); + + hostBridge().emit('setHighlightsVisible', false); + guestBridge().emit('connect', fakeChannel); + assert.calledWith(fakeChannel.call, 'setHighlightsVisible', false); + }); }); context('when a frame is destroyed', () => { @@ -508,10 +525,10 @@ describe('FrameSyncService', () => { assert.calledWith(hostBridge().call, 'closeSidebar'); }); - it('calls "setVisibleHighlights"', () => { - hostBridge().emit('setVisibleHighlights'); + it('calls "setHighlightsVisible"', () => { + hostBridge().emit('setHighlightsVisible'); - assert.calledWith(guestBridge().call, 'setVisibleHighlights'); + assert.calledWith(guestBridge().call, 'setHighlightsVisible'); }); });