Commit f6ac11fc authored by Robert Knight's avatar Robert Knight

Make sidebar app responsible for opening sidebar to edit draft

Move the responsibility for opening the sidebar when there is a draft to
edit from the guest into the sidebar's `FrameSyncService`.

The initial motivation for this was to make the logic in the
`beforeAnnotationCreated` event handler in the `Sidebar` class work if
the annotation is created in a non-host guest frame. The plan was to
make the sidebar send an `openSidebar` request with custom options that
would trigger the logic that used to be in the `beforeAnnotationCreated`
event handler.

However it appears that focusing the sidebar frame is no longer necessary. In my
testing the input field in the sidebar was successfully focused regardless
of whether `iframe.contentWindow.focus()` was called or not. Therefore I
have just removed the handler.

Even though the original need no longer exists, moving the logic still
seems sensible because logically it is the sidebar application that
knows it is awaiting user input after a new draft is created. It also means that
if we do find a need to re-introduce the frame-focusing logic that used
to be in the `beforeAnnotationCreated` handler, we can do so in a way
that works with annotations created in non-host frames.
parent f484236b
...@@ -576,10 +576,6 @@ export default class Guest { ...@@ -576,10 +576,6 @@ export default class Guest {
this._emitter.publish('beforeAnnotationCreated', annotation); this._emitter.publish('beforeAnnotationCreated', annotation);
this.anchor(annotation); this.anchor(annotation);
if (!annotation.$highlight) {
this._bridge.call('openSidebar');
}
return annotation; return annotation;
} }
......
...@@ -110,15 +110,6 @@ export default class Sidebar { ...@@ -110,15 +110,6 @@ export default class Sidebar {
this._listeners = new ListenerCollection(); this._listeners = new ListenerCollection();
this._emitter.subscribe('beforeAnnotationCreated', annotation => {
// 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
if (!annotation.$highlight) {
/** @type {Window} */ (this.iframe.contentWindow).focus();
}
});
// Set up the toolbar on the left edge of the sidebar. // Set up the toolbar on the left edge of the sidebar.
const toolbarContainer = document.createElement('div'); const toolbarContainer = document.createElement('div');
this.toolbar = new ToolbarController(toolbarContainer, { this.toolbar = new ToolbarController(toolbarContainer, {
......
...@@ -805,18 +805,6 @@ describe('Guest', () => { ...@@ -805,18 +805,6 @@ describe('Guest', () => {
assert.equal(annotation.$highlight, true); assert.equal(annotation.$highlight, true);
}); });
it('opens sidebar if `highlight` is false', async () => {
const guest = createGuest();
await guest.createAnnotation();
assert.calledWith(fakeBridge.call, 'openSidebar');
});
it('does not open sidebar if `highlight` is true', async () => {
const guest = createGuest();
await guest.createAnnotation({ highlight: true });
assert.notCalled(fakeBridge.call);
});
it('triggers a "beforeAnnotationCreated" event', async () => { it('triggers a "beforeAnnotationCreated" event', async () => {
const guest = createGuest(); const guest = createGuest();
const callback = sandbox.stub(); const callback = sandbox.stub();
......
...@@ -232,37 +232,6 @@ describe('Sidebar', () => { ...@@ -232,37 +232,6 @@ describe('Sidebar', () => {
); );
}); });
context('when a new annotation is created', () => {
function stubIframeWindow(sidebar) {
const iframe = sidebar.iframe;
const fakeIframeWindow = { focus: sinon.stub() };
sinon.stub(iframe, 'contentWindow').get(() => fakeIframeWindow);
return iframe;
}
it('focuses the sidebar if the annotation is not a highlight', () => {
const sidebar = createSidebar();
const iframe = stubIframeWindow(sidebar);
sidebar._emitter.publish('beforeAnnotationCreated', {
$highlight: false,
});
assert.called(iframe.contentWindow.focus);
});
it('does not focus the sidebar if the annotation is a highlight', () => {
const sidebar = createSidebar();
const iframe = stubIframeWindow(sidebar);
sidebar._emitter.publish('beforeAnnotationCreated', {
$highlight: true,
});
assert.notCalled(iframe.contentWindow.focus);
});
});
describe('toolbar buttons', () => { describe('toolbar buttons', () => {
it('opens and closes sidebar when toolbar button is clicked', () => { it('opens and closes sidebar when toolbar button is clicked', () => {
const sidebar = createSidebar(); const sidebar = createSidebar();
......
...@@ -150,6 +150,12 @@ export class FrameSyncService { ...@@ -150,6 +150,12 @@ export class FrameSyncService {
} }
inFrame.add(event.tag); inFrame.add(event.tag);
// Open the sidebar so that the user can immediately edit the draft
// annotation.
if (!annot.$highlight) {
this._hostRPC.call('openSidebar');
}
// Create the new annotation in the sidebar. // Create the new annotation in the sidebar.
annotationsService.create(annot); annotationsService.create(annot);
}); });
......
...@@ -326,6 +326,26 @@ describe('FrameSyncService', () => { ...@@ -326,6 +326,26 @@ describe('FrameSyncService', () => {
}) })
); );
}); });
it('opens the sidebar ready for the user to edit the draft', () => {
frameSync.connect();
fakeStore.isLoggedIn.returns(true);
const ann = { target: [] };
guestBridge().emit('beforeCreateAnnotation', { tag: 't1', msg: ann });
assert.calledWith(hostBridge().call, 'openSidebar');
});
it('does not open the sidebar if the annotation is a highlight', () => {
frameSync.connect();
fakeStore.isLoggedIn.returns(true);
const ann = { $highlight: true, target: [] };
guestBridge().emit('beforeCreateAnnotation', { tag: 't1', msg: ann });
assert.notCalled(hostBridge().call);
});
}); });
context('when no authenticated user is present', () => { context('when no authenticated user is present', () => {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment