Use separate channels for guest / host messages from sidebar #3807
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3807 +/- ##
==========================================
- Coverage 99.01% 98.98% -0.03%
==========================================
Files 212 212
Lines 7796 7802 +6
Branches 1757 1757
==========================================
+ Hits 7719 7723 +4
- Misses 77 79 +2
Continue to review full report at Codecov.
|
Use separate channels for sending messages to guests vs the host in the sidebar, even for the common case when there is only one guest and it is the same frame as the host. This change makes it clear for readers which part of the annotator code is intended to handle a particular message from the sidebar. It is also a step towards supporting host frames that are not guests. This will be needed in ebook readers where the host frame provides the navigation UI and contains the frame displaying the book content, but should not be annotatble itself. - Remove the `bridge` service in the sidebar. The `frameSync` service now provides the entry point for other services/components to make RPC calls to the host or guest frames. Currently the only use case is sending notifications to the host via `FrameSyncService.notifyHost`. - Create separate `Bridge` instances in `FrameSyncService` for sidebar <-> guest and sidebar <-> host communication. The sidebar <-> guest bridge works the same as before. The sidebar <-> host bridge is established by having `FrameSyncService` create a MessageChannel when sending the `hypothesisSidebarReady` notification to the host. The sidebar / host frames then add respective ports of this channel to a Bridge instance. - Change the various existing RPC calls between frames to use either the guest <-> sidebar or host <-> sidebar communication channels as appropriate
robertknight
force-pushed
the
split-guest-host-messages
branch
from
fe6d26f to
711ca92
Compare
There was a problem hiding this comment.
I don't see any problems with the code.
I find a little confusing that the host-sidebar channel is created in the sidebar while the guest-sidebar for the host frame is created in the annotator. I think it would be better to have one frame to be responsible of the creation of all the MessageChannels.
Because it is imposible to reach the the sidebar or the host (except window.parent) from a cross-origin frame I think the host frame is the ideal place to create all the channels (some on demand) and orchestrate the communication.
I planned on an alternative way to send events to specific channels without involving creating multiple Bridges instances. This involved specifying the role of that frame when it is registered and then directing calls to specific frames: this.bridge.call('myevent', {args, to: [annotatables, notebook]}) (to send messages to all the annotatables and notebook frames. (see for example: #3566 (comment))
There is a missing test for this line:
src/sidebar/services/frame-sync.js#L294-L295
| @@ -199,8 +202,9 @@ export default class Sidebar { | |||
| */ | |||
| this.ready = new Promise(resolve => { | |||
| this._listeners.add(window, 'message', event => { | |||
| const data = /** @type {MessageEvent} */ (event).data; | |||
| if (data?.type === 'hypothesisSidebarReady') { | |||
| const messageEvent = /** @type {MessageEvent} */ (event); | |||
There was a problem hiding this comment.
| const messageEvent = /** @type {MessageEvent} */ (event); | |
| const {data, ports} = /** @type {MessageEvent} */ (event); |
There was a problem hiding this comment.
We need to allow for the fact that data may be null/undefined here in case code on the host page has somehow sent an message event with a null data value.
| /** Channel for sidebar <-> guest(s) communication. */ | ||
| this._guestRPC = new Bridge(); |
There was a problem hiding this comment.
Something to consider for the future, maybe we should rename this sidebar-annotatable channel to fully differentiate that it is not the intention to send messages to any guests frame but annotatable frames.
There was a problem hiding this comment.
To me "guest frame" and "annotatable frame" means the same. What distinction were you thinking of?
| inFrame.delete(annot.$tag); | ||
| }); | ||
|
|
||
| if (frames.length > 0) { | ||
| if (frames.every(frame => frame.isAnnotationFetchComplete)) { | ||
| if (publicAnns === 0 || publicAnns !== prevPublicAnns) { | ||
| bridge.call( | ||
| this._hostRPC.call( |
There was a problem hiding this comment.
Out of curiosity, is there something in the host that reacts to the publicAnnotationCountChanged event?
There was a problem hiding this comment.
Yes. It supports a feature where the publisher can display the count of annotations on their site. See https://h.readthedocs.io/projects/client/en/latest/publishers/host-page-integration/#cmdoption-arg-data-hypothesis-annotation-count.
There was a problem hiding this comment.
Thanks, very good to know.
| * Query the Hypothesis annotation client in a frame for the URL and metadata | ||
| * of the document that is currently loaded and add the result to the set of | ||
| * connected frames. | ||
| * Query the guest in a frame for the URL and metadata of the document that |
There was a problem hiding this comment.
| * Query the guest in a frame for the URL and metadata of the document that | |
| * Query the annotatable frame for the URL and metadata of the document that |
| */ | ||
| this._setupSyncToFrame = () => { | ||
| this._setupSyncToGuests = () => { |
There was a problem hiding this comment.
| this._setupSyncToGuests = () => { | |
| this._setupSyncToAnnotatables = () => { |
| * to the sidebar. | ||
| */ | ||
| this._setupSyncFromFrame = () => { | ||
| this._setupSyncFromGuests = () => { |
There was a problem hiding this comment.
| this._setupSyncFromGuests = () => { | |
| this._setupSyncFromAnnotatables = () => { |
| function sendSidebarReadyMessage() { | ||
| const channel = new MessageChannel(); | ||
| const event = new MessageEvent( | ||
| 'message', | ||
| { | ||
| data: { type: 'hypothesisSidebarReady' }, | ||
| }, | ||
| [channel.port1] | ||
| ); | ||
|
|
||
| window.dispatchEvent(event); | ||
|
|
||
| return event; | ||
| } | ||
|
|
||
| it('creates Bridge for host <-> sidebar RPC calls when `hypothesisSidebarReady` message is received', () => { | ||
| createSidebar(); | ||
| const event = sendSidebarReadyMessage(); | ||
| assert.calledWith(fakeBridge.createChannel, event.ports[0]); | ||
| }); |
There was a problem hiding this comment.
An alternative...
| function sendSidebarReadyMessage() { | |
| const channel = new MessageChannel(); | |
| const event = new MessageEvent( | |
| 'message', | |
| { | |
| data: { type: 'hypothesisSidebarReady' }, | |
| }, | |
| [channel.port1] | |
| ); | |
| window.dispatchEvent(event); | |
| return event; | |
| } | |
| it('creates Bridge for host <-> sidebar RPC calls when `hypothesisSidebarReady` message is received', () => { | |
| createSidebar(); | |
| const event = sendSidebarReadyMessage(); | |
| assert.calledWith(fakeBridge.createChannel, event.ports[0]); | |
| }); | |
| function sendSidebarReadyMessage() { | |
| const { port1 } = new MessageChannel(); | |
| window.postMessage({ type: 'hypothesisSidebarReady' }, '*', [port1]); | |
| return port1; | |
| } | |
| it('creates Bridge for host <-> sidebar RPC calls when `hypothesisSidebarReady` message is received', async () => { | |
| createSidebar(); | |
| const port = sendSidebarReadyMessage(); | |
| await delay(0); | |
| assert.calledWith(fakeBridge.createChannel, port); | |
| }); |
There was a problem hiding this comment.
I did try something like this initially, but I realized I wasn't sure what the guarantees were around how long/when postMessage delivers its message, so I figured just dispatching the message directly was simpler and "realistic enough" for this case.
The main goal of this PR is to establish separate channels for sidebar <-> host and sidebar <-> guest communication and to separate which messages need to be sent via which channel. I fully expect we might want to change how and where the creation of the channels happen in future. Regarding this comment:
Hmm... we need to use consistent terminology for "a frame which the user can annotate". I thought we had already settled on "guest frame" or "guest" for short, mainly because this means it contains an instance of the |
I have added the missing test. I've left the "guest" terminology as-is for the moment. Let's catch up about that tomorrow or Monday as we need to be consistent about it. |
I have no problem referring to annotatable frames as
|
Follow up of #3807 * reference to the `crossframe` service is inaccurate * changed default export to a named export, as per team conventions
Follow up of #3807 * reference to the `crossframe` service is inaccurate * changed default export to a named export, as per team conventions
Follow up of #3807 * reference to the `crossframe` service is inaccurate * changed default export to a named export, as per team conventions
Follow up of #3807 * reference to the `crossframe` service is inaccurate * changed default export to a named export, as per team conventions
Follow up of #3807 * reference to the `crossframe` service is inaccurate * changed default export to a named export, as per team conventions
Follow up of #3807 * reference to the `crossframe` service is inaccurate * changed default export to a named export, as per team conventions
Follow up of #3807 * reference to the `crossframe` service is inaccurate * changed default export to a named export, as per team conventions
Use separate channels for sending messages to guests vs the host in the
sidebar, even for the common case when there is only one guest and it is
the same frame as the host.
This change makes it clear for readers which part of the annotator code
is intended to handle a particular message from the sidebar. It is also
a step towards supporting host frames that are not guests. This will be
needed in ebook readers where the host frame provides the navigation UI
and contains the frame displaying the book content, but should not be
annotatable itself.
Remove the
bridgeservice in the sidebar. TheframeSyncservicenow provides the entry point for other services/components to make RPC calls
to the host or guest frames. Currently the only use case is
sending notifications to the host via
FrameSyncService.notifyHost.Create separate
Bridgeinstances inFrameSyncServicefor sidebar<-> guest and sidebar <-> host communication. The sidebar <-> guest
bridge works the same as before. The sidebar <-> host bridge
is established by having
FrameSyncServicecreate a MessageChannelwhen sending the
hypothesisSidebarReadynotification to the host.The sidebar / host frames then add respective ports of this channel
to a Bridge instance.
Change the various existing RPC calls between frames to use either
the guest <-> sidebar or host <-> sidebar communication channels as
appropriate