Fix ignoring of events inside Hypothesis UI elements #6717
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `maybeCloseSidebar` function inside guest.ts relied on events inside
Hypothesis UI elements not reaching it because they were stopped at the shadow
root boundary by the `stopEventPropagation` helper in shadow-root.ts. However
this was broken because `maybeCloseSidebar` is called in response to
`pointerdown` events but `stopEventPropagation` does not intercept this event.
The issue usually went unnoticed because clicking in the sidebar's toggle
button when it was closed resulted in the following sequence of events:
1. "pointerdown" event, processed by `maybeCloseSidebar`. This sent a redundant
"closeSidebar" message to the sidebar when it was already closed.
2. "click" event handled, resulting in the sidebar opening.
In some circumstances however, such as when tap-to-click is enabled on macOS [1],
these events were sometimes received in the opposite order. As a result the
sidebar was opened and then immediately closed.
Simplify the approach by removing the logic for preventing event propagation and
instead testing whether the element is a Hypothesis UI element inside
`maybeCloseSidebar` by looking at its tag name.
[^1]: https://hypothes-is.slack.com/archives/C4K6M7P5E/p1733756425323569
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6717 +/- ##
==========================================
- Coverage 99.43% 99.42% -0.01%
==========================================
Files 271 271
Lines 10215 10214 -1
Branches 2434 2437 +3
==========================================
- Hits 10157 10155 -2
- Misses 58 59 +1 ☔ View full report in Codecov by Sentry. |
robertknight
commented
Dec 9, 2024
| * | ||
| * This makes the host page a little bit less aware of the annotator activity. | ||
| * It is still possible for the host page to manipulate the events on the capturing | ||
| * face. |
There was a problem hiding this comment.
This comment is hand-wavy and doesn't point to any specific issues with web pages that we need to be aware of, so I'm going to ignore it. I will look back through the commit history tomorrow in case I can find any remaining reasons to keep the logic, but otherwise let's try to simplify the code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
maybeCloseSidebarfunction inside guest.ts relied on events inside Hypothesis UI elements not reaching it because they were stopped at the shadow root boundary by thestopEventPropagationhelper in shadow-root.ts. However this was broken becausemaybeCloseSidebaris called in response topointerdownevents bystopEventPropagationdoes not intercept this event.The issue usually went unnoticed because clicking in the sidebar's toggle button when it was closed resulted in the following sequence of events:
maybeCloseSidebar. This sent a redundant "closeSidebar" message to the sidebar when it was already closed.In some circumstances however, such as when tap-to-click is enabled on macOS [1], these events were sometimes received in the opposite order. As a result the sidebar was opened and then immediately closed.
Simplify the approach by removing the logic for preventing event propagation and instead testing whether the element is a Hypothesis UI element inside
maybeCloseSidebarby looking at its tag name.[1] https://hypothes-is.slack.com/archives/C4K6M7P5E/p1733756425323569