Skip to content

Commit

Permalink
Make showing highlights when creating an annotation work for all guests
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
robertknight committed Oct 14, 2021
1 parent f7f2dc7 commit 94fb864
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 1 deletion.
4 changes: 3 additions & 1 deletion src/annotator/sidebar.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,6 @@ 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();
}
Expand Down Expand Up @@ -245,6 +244,9 @@ export default class Sidebar {
sidebarTrigger(document.body, () => this.open());
features.init(this._sidebarRPC);

this._sidebarRPC.on('showHighlights', () =>
this.setHighlightsVisible(true)
);
this._sidebarRPC.on('openSidebar', () => this.open());
this._sidebarRPC.on('closeSidebar', () => this.close());

Expand Down
9 changes: 9 additions & 0 deletions src/annotator/test/sidebar-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,15 @@ describe('Sidebar', () => {
return result;
};

describe('on "showHighlights" event', () => {
it('makes all highlights visible', () => {
createSidebar();
assert.isFalse(fakeToolbar.highlightsVisible);
emitEvent('showHighlights');
assert.isTrue(fakeToolbar.highlightsVisible);
});
});

describe('on "open" event', () =>
it('opens the frame', () => {
const target = sandbox.stub(Sidebar.prototype, 'open');
Expand Down
5 changes: 5 additions & 0 deletions src/sidebar/services/frame-sync.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,11 @@ export class FrameSyncService {
}
inFrame.add(event.tag);

// Ensure that the highlight for the newly-created annotation is visible.
// Currently we only support a single, shared visibility state for all highlights
// in all frames, so this will make all existing highlights visible too.
this._hostRPC.call('showHighlights');

// Create the new annotation in the sidebar.
annotationsService.create(annot);
});
Expand Down
10 changes: 10 additions & 0 deletions src/sidebar/services/test/frame-sync-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,16 @@ describe('FrameSyncService', () => {
});

context('when a new annotation is created in the frame', () => {
it('makes the new highlight visible in the frame', () => {
frameSync.connect();
fakeStore.isLoggedIn.returns(true);
const ann = { target: [] };

guestBridge().emit('beforeCreateAnnotation', { tag: 't1', msg: ann });

assert.calledWith(hostBridge().call, 'showHighlights');
});

context('when an authenticated user is present', () => {
it('creates the annotation in the sidebar', () => {
frameSync.connect();
Expand Down

0 comments on commit 94fb864

Please sign in to comment.