Skip to content

Commit 47e35ef

Browse files
committed
Make showing highlights when creating an annotation work for all guests
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.
1 parent 8e8651a commit 47e35ef

File tree

4 files changed

+27
-1
lines changed

4 files changed

+27
-1
lines changed

src/annotator/sidebar.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,6 @@ export default class Sidebar {
114114
// When a new non-highlight annotation is created, focus
115115
// the sidebar so that the text editor can be focused as
116116
// soon as the annotation card appears
117-
this.setHighlightsVisible(true);
118117
if (!annotation.$highlight) {
119118
/** @type {Window} */ (this.iframe.contentWindow).focus();
120119
}
@@ -243,6 +242,9 @@ export default class Sidebar {
243242
sidebarTrigger(document.body, () => this.open());
244243
features.init(this._sidebarRPC);
245244

245+
this._sidebarRPC.on('showHighlights', () =>
246+
this.setHighlightsVisible(true)
247+
);
246248
this._sidebarRPC.on('openSidebar', () => this.open());
247249
this._sidebarRPC.on('closeSidebar', () => this.close());
248250

src/annotator/test/sidebar-test.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,6 +328,15 @@ describe('Sidebar', () => {
328328
return result;
329329
};
330330

331+
describe('on "showHighlights" event', () => {
332+
it('makes all highlights visible', () => {
333+
createSidebar();
334+
assert.isFalse(fakeToolbar.highlightsVisible);
335+
emitEvent('showHighlights');
336+
assert.isTrue(fakeToolbar.highlightsVisible);
337+
});
338+
});
339+
331340
describe('on "open" event', () =>
332341
it('opens the frame', () => {
333342
const target = sandbox.stub(Sidebar.prototype, 'open');

src/sidebar/services/frame-sync.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,11 @@ export class FrameSyncService {
153153
}
154154
inFrame.add(event.tag);
155155

156+
// Ensure that the highlight for the newly-created annotation is visible.
157+
// Currently we only support a single, shared visibility state for all highlights
158+
// in all frames, so this will make all existing highlights visible too.
159+
this._hostRPC.call('showHighlights');
160+
156161
// Create the new annotation in the sidebar.
157162
annotationsService.create(annot);
158163
});

src/sidebar/services/test/frame-sync-test.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,16 @@ describe('FrameSyncService', () => {
310310
});
311311

312312
context('when a new annotation is created in the frame', () => {
313+
it('makes the new highlight visible in the frame', () => {
314+
frameSync.connect();
315+
fakeStore.isLoggedIn.returns(true);
316+
const ann = { target: [] };
317+
318+
guestBridge().emit('beforeCreateAnnotation', { tag: 't1', msg: ann });
319+
320+
assert.calledWith(hostBridge().call, 'showHighlights');
321+
});
322+
313323
context('when an authenticated user is present', () => {
314324
it('creates the annotation in the sidebar', () => {
315325
frameSync.connect();

0 commit comments

Comments
 (0)