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.

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

Fix ignoring of events inside Hypothesis UI elements #6717

Merged
merged 1 commit into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*
* 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);
});
});
});
Loading