Skip to content

Commit

Permalink
Fix ignoring of events inside Hypothesis UI elements
Browse files Browse the repository at this point in the history
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
  • Loading branch information
robertknight committed Dec 10, 2024
1 parent c9e9d6d commit e763a9a
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 73 deletions.
19 changes: 14 additions & 5 deletions src/annotator/guest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -365,18 +365,27 @@ export class Guest extends TinyEmitter implements Annotator, Destroyable {
_setupElementEvents() {
// Hide the sidebar in response to a document click or tap, so it doesn't obscure
// the document content.
//
// Hypothesis UI elements (`<hypothesis->`) have logic to prevent clicks in
// them from propagating out of their shadow roots, and hence clicking on
// elements in the sidebar's vertical toolbar or adder won't close the
// sidebar.
const maybeCloseSidebar = (event: PointerEvent) => {
// Don't hide the sidebar if event was disabled because the sidebar
// doesn't overlap the content.
if (this._sideBySideActive()) {
return;
}

// Don't hide the sidebar if clicking inside a `<hypothesis-*>` UI
// element. This includes the controls that open and close the sidebar.
if (
event
.composedPath()
.some(
target =>
target instanceof Element &&
target.localName.startsWith('hypothesis-'),
)
) {
return;
}

// Don't hide the sidebar if the event comes from an element that contains a highlight
if (annotationsAt(event.target as Element).length) {
return;
Expand Down
20 changes: 20 additions & 0 deletions src/annotator/test/guest-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,26 @@ describe('Guest', () => {

assert.isFalse(sidebarClosed());
});

it('does not hide sidebar if event is inside a `<hypothesis-*>` element', () => {
createGuest();

const hypothesisElement = document.createElement('hypothesis-sidebar');
const nonHypothesisElement = document.createElement('other-element');

try {
rootElement.append(hypothesisElement, nonHypothesisElement);

simulateClick(hypothesisElement);
assert.isFalse(sidebarClosed());

simulateClick(nonHypothesisElement);
assert.isTrue(sidebarClosed());
} finally {
hypothesisElement.remove();
nonHypothesisElement.remove();
}
});
});

it('does not reposition the adder if hidden when the window is resized', () => {
Expand Down
20 changes: 0 additions & 20 deletions src/annotator/util/shadow-root.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,25 +36,5 @@ export function createShadowRoot(container: HTMLElement): ShadowRoot {
applyFocusVisible(shadowRoot);
}

stopEventPropagation(shadowRoot);
return shadowRoot;
}

/**
* Stop bubbling up of several events.
*
* 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.
*
* Another benefit is that click and touchstart typically causes the sidebar to close.
* By preventing the bubble up of these events, we don't have to individually stop
* the propagation.
*/
function stopEventPropagation(element: HTMLElement | ShadowRoot) {
element.addEventListener('mouseup', event => event.stopPropagation());
element.addEventListener('mousedown', event => event.stopPropagation());
element.addEventListener('touchstart', event => event.stopPropagation(), {
passive: true,
});
}
48 changes: 0 additions & 48 deletions src/annotator/util/test/shadow-root-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,53 +60,5 @@ describe('annotator/util/shadow-root', () => {
assert.isNull(linkEl);
link.setAttribute('rel', 'stylesheet');
});

it('stops propagation of "mouseup" events', () => {
const onClick = sinon.stub();
container.addEventListener('click', onClick);

const shadowRoot = createShadowRoot(container);
const innerElement = document.createElement('div');
shadowRoot.appendChild(innerElement);
innerElement.dispatchEvent(
// `composed` property is necessary to bubble up the event out of the shadow DOM.
// browser generated events, have this property set to true.
new Event('mouseup', { bubbles: true, composed: true }),
);

assert.notCalled(onClick);
});

it('stops propagation of "mousedown" events', () => {
const onClick = sinon.stub();
container.addEventListener('mousedown', onClick);

const shadowRoot = createShadowRoot(container);
const innerElement = document.createElement('div');
shadowRoot.appendChild(innerElement);
innerElement.dispatchEvent(
// `composed` property is necessary to bubble up the event out of the shadow DOM.
// browser generated events, have this property set to true.
new Event('mousedown', { bubbles: true, composed: true }),
);

assert.notCalled(onClick);
});

it('stops propagation of "touchstart" events', () => {
const onTouch = sinon.stub();
container.addEventListener('touchstart', onTouch);

const shadowRoot = createShadowRoot(container);
const innerElement = document.createElement('div');
shadowRoot.appendChild(innerElement);
// `composed` property is necessary to bubble up the event out of the shadow DOM.
// browser generated events, have this property set to true.
innerElement.dispatchEvent(
new Event('touchstart', { bubbles: true, composed: true }),
);

assert.notCalled(onTouch);
});
});
});

0 comments on commit e763a9a

Please sign in to comment.