Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issues with guest frame support that impact VitalSource #3798

Closed
5 tasks done
robertknight opened this issue Oct 1, 2021 · 4 comments
Closed
5 tasks done

Issues with guest frame support that impact VitalSource #3798

robertknight opened this issue Oct 1, 2021 · 4 comments
Labels
VitalSource

Comments

@robertknight
Copy link
Member

robertknight commented Oct 1, 2021 •
edited
Loading

This is a partial list of issues with the client's current support for guest frames which impact VitalSource, and that we'll need to solve.

  1. The current FrameObserver/HypothesisInjector system for adding the client to iframes on the page does not support discovering iframes that are contained in shadow roots. In the VitalSource book reader, the content frame is contained in a shadow root attached to a <mosaic-book> element. (Update: Resolved by exposing the ability for an integration to inject the client into a specific frame. See Implement "manual" injection of client into frames when requested by integration #3813)
  2. If a guest iframe is unloaded, the sidebar is not notified (via a destroyFrame RPC call) unless that frame is monitored by FrameObserver. The obvious thing to do here would be for the Guest class to notify the sidebar when destroyed, and to call guest.destroy() from the window.unload event in the guest iframe. In my testing this works in Chrome and Firefox but I encountered a problem in Safari where MessagePort messages posted from this event handler don't seem to arrive in the destination frame. (Update: Fixed by Route guest frame unload notifications via host frame #3812)
  3. The sidebar currently assumes that there is one "main" frame with no frame identifier and one or more sub-frames. Various parts of the UI, such as the Help panel, reflect only the URL from the main frame. In ebook readers we typically don't want to allow annotation in the host frame but only the content frame. (Update: Partially resolved. The Help panel now displays URLs from all connected frames. See Show URLs for all connected frames in help panel #3811)
  4. If multiple frames are connected to the sidebar, annotations can incorrectly be marked as orphans because the sidebar sends all loaded annotations to all frames, instead of only sending them to the frame with the corresponding document. (Inconsistent re-anchoring in multi-guest scenarios #3992) (Update: This has been fixed for VitalSource by not injecting the client into the host frame, so the only guest is the book content, and by only sending annotations to a single frame in FrameSyncService. There are still things to fix in the general case outside of VS, see WIP - Fetch annotations for each frame separately #4193.)
  5. Injection of the client by HypothesisInjector into iframes on the page doesn't work in the browser extension, because the assetRoot configuration setting is not injected. (Update: Fixed by Proxy asset urls from host frame into injected iframes #3814)

For issues (1) and (2), a possible solution might be to have some way for the document-type integration to signal that a particular frame should have the client injected, even if it would not have been auto-discovered by FrameObserver.

For issues (3) and (4), we'll need some way to signal to the sidebar that the top-level (host frame) should be ignored and not stored in the frames state. One way to do this would be to avoid instantiating the Guest class in that frame. However the annotator code currently has no support for host frames without guest functionality. So we'll either need to add that, or provide a way to signal that the guest functionality is inactive for a particular frame.
@robertknight
Copy link
Member Author

Various parts of the UI, such as the Help panel, reflect only the URL from the main frame.

Remaining references to the mainFrame store selector in the UI:

  1. Group fetch: Used to fetch groups associated with the current page.
  2. Share annotations panel: Used to generate a share URI for the document
  3. Annotations service: Used to handle user clicking "Create note" button on the "Page Notes" tab

In cases (1) and (2) it would probably make most sense to use the URL of the host page.

In case (3) we need to decide which of the connected guests the note should correspond to. For the common guest of a host frame which is also the only guest, it should be that one. In an ebook reader it should be the frame that contains the book content. In cases where there are really multiple guests we need to pick one as the main frame.

@robertknight
Copy link
Member Author

robertknight commented Oct 14, 2021 •
edited
Loading

Some additional things about guest frame support that affect ebook readers, including VitalSource. In these notes I use the term "host-guest" to refer to the Guest instance created in the host frame where the client is initially loaded, and which is passed to the Sidebar constructor:

  1. The bucket bar only displays anchors from the host-guest
  2. The initial un-hiding of the sidebar after the client loads is triggered by a panelReady event which is emitted by the host-guest after it connects to the sidebar (Replace panelReady event for triggering initial sidebar display #3831)
  3. The "Create annotation / Page note" button in the sidebar's vertical controls requests the host-guest to create an annotation
  4. The "Create annotation / Page note" button in the sidebar's vertical controls changes to reflect the state of the selection in the host-guest
  5. The sidebar directly toggles highlight visibility in the host-guest when the sidebar is opened and closed (Make highlight visibility state work in a multi-guest world #3830)
  6. The sidebar calls the host-guest's fitSideBySide method when the sidebar is opened or closed
  7. The host-guest changes the state of the "Highlights visible" button in the sidebar's controls (Make highlight visibility state work in a multi-guest world #3830)
  8. The sidebar listens for the beforeAnnotationCreated event in the host-guest in order to focus the sidebar window when a new annotation is created (Make sidebar app responsible for opening sidebar to edit draft #3832)

In order to support the scenarios where there are multiple guests, or a single guest that is not the host frame and changes as the user navigates in the book, we're going to need to address each of these.

robertknight added a commit that referenced this issue Oct 14, 2021
@robertknight
Replace panelReady event for triggering initial sidebar display
d32e293 
The initial un-hiding and opening of the sidebar was previously
triggered by a `panelReady` event emitted by the Guest in the host frame
when it connects.

As part of eliminating the need for the host frame to also be a guest,
change the initial sidebar open to use the `Sidebar.ready` promise
instead, which is resolved when the sidebar applicaiton sends a
`hypothesisSidebarReady` notice to the `Sidebar` app.

Part of #3798
robertknight added a commit that referenced this issue Oct 14, 2021
@robertknight
Replace panelReady event for triggering initial sidebar display
da9ff44 
The initial un-hiding and opening of the sidebar was previously
triggered by a `panelReady` event emitted by the Guest in the host frame
when it connects.

As part of eliminating the need for the host frame to also be a guest,
change the initial sidebar open to use the `Sidebar.ready` promise
instead, which is resolved when the sidebar applicaiton sends a
`hypothesisSidebarReady` notice to the `Sidebar` app.

Part of #3798
robertknight added a commit that referenced this issue Oct 14, 2021
@robertknight
Make highlight visibility setting work in a multi-guest world
01c33d5 
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 #3798
robertknight added a commit that referenced this issue Oct 14, 2021
@robertknight
Make highlight visibility setting work in a multi-guest world
f7f2dc7 
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 #3798
This was referenced Oct 14, 2021
Merged
Merged
Merged
Closed
robertknight added a commit that referenced this issue Oct 19, 2021
@robertknight
Replace panelReady event for triggering initial sidebar display
f484236 
The initial un-hiding and opening of the sidebar was previously
triggered by a `panelReady` event emitted by the Guest in the host frame
when it connects.

As part of eliminating the need for the host frame to also be a guest,
change the initial sidebar open to use the `Sidebar.ready` promise
instead, which is resolved when the sidebar applicaiton sends a
`hypothesisSidebarReady` notice to the `Sidebar` app.

Part of #3798
robertknight added a commit that referenced this issue Oct 19, 2021
@robertknight
Make highlight visibility setting work in a multi-guest world
8e8651a 
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 #3798
robertknight added a commit that referenced this issue Oct 19, 2021
@robertknight
Make highlight visibility setting work in a multi-guest world
e3b7728 
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 #3798
robertknight added a commit that referenced this issue Oct 19, 2021
@robertknight
Make highlight visibility setting work in a multi-guest world
7d5ff32 
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 #3798
@robertknight
Copy link
Member Author

Relevant discussion on Slack: https://hypothes-is.slack.com/archives/C1M8NH76X/p1635426991028600

This was referenced Dec 6, 2021
Closed
Closed
@robertknight
Copy link
Member Author

I'm going to close this now as all of the critical issues with guest frame support that affect VS have been fixed. There is still an important issue with guest frame support outside of VS (see #4193).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
VitalSource
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@robertknight