Commit d9de7bde authored by Robert Knight's avatar Robert Knight

Install shortcut listener on document element

Listen for key events for keyboard shortcuts on the document element rather than
body element, to make them work in XHTML documents.

When no interactive element (eg. an input field) is focused in an HTML document,
keyboard events are sent to the body element. In an XHTML document however
keyboard events are sent to the document element instead in Safari and Chrome
(in Firefox they are still sent to the body). The key event listeners used for
the adder's shortcuts were installed on the body element, so they didn't work in
XHTML documents in Safari and Chrome.

My guess is that the reason for this behavioral quirk is that HTML documents are
guaranteed to have a body element - an empty one will be generated if none
is present in the markup, whereas XHTML documents are not.

Fixes https://github.com/hypothesis/client/issues/4364
parent 896086a4
...@@ -80,7 +80,9 @@ describe('Adder', () => { ...@@ -80,7 +80,9 @@ describe('Adder', () => {
getContent(adder).querySelector(`button[title^="${label}"]`); getContent(adder).querySelector(`button[title^="${label}"]`);
const triggerShortcut = key => const triggerShortcut = key =>
document.body.dispatchEvent(new KeyboardEvent('keydown', { key })); document.body.dispatchEvent(
new KeyboardEvent('keydown', { key, bubbles: true })
);
const showAdder = () => { const showAdder = () => {
// nb. `act` is necessary here to flush effect hooks in `AdderToolbar` // nb. `act` is necessary here to flush effect hooks in `AdderToolbar`
......
...@@ -80,7 +80,13 @@ export function matchShortcut(event, shortcut) { ...@@ -80,7 +80,13 @@ export function matchShortcut(event, shortcut) {
export function installShortcut( export function installShortcut(
shortcut, shortcut,
onPress, onPress,
{ rootElement = document.body } = {} {
// We use `documentElement` as the root element rather than `document.body`
// which is used as a root element in some other places because the body
// element is not keyboard-focusable in XHTML documents in Safari/Chrome.
// See https://github.com/hypothesis/client/issues/4364.
rootElement = document.documentElement,
} = {}
) { ) {
/** @param {KeyboardEvent} event */ /** @param {KeyboardEvent} event */
const onKeydown = event => { const onKeydown = event => {
...@@ -108,11 +114,7 @@ export function installShortcut( ...@@ -108,11 +114,7 @@ export function installShortcut(
* @param {(e: KeyboardEvent) => any} onPress - A function to call when the shortcut matches * @param {(e: KeyboardEvent) => any} onPress - A function to call when the shortcut matches
* @param {ShortcutOptions} [options] * @param {ShortcutOptions} [options]
*/ */
export function useShortcut( export function useShortcut(shortcut, onPress, { rootElement } = {}) {
shortcut,
onPress,
{ rootElement = document.body } = {}
) {
useEffect(() => { useEffect(() => {
if (!shortcut) { if (!shortcut) {
return undefined; return undefined;
......
...@@ -84,13 +84,26 @@ describe('shared/shortcut', () => { ...@@ -84,13 +84,26 @@ describe('shared/shortcut', () => {
}); });
}); });
/**
* Simulate a key press on a document.
*
* In an HTML document this will go to the body if no other element is
* focused. In an XHTML document this will go to the document element in
* Safari and Chrome. In both cases the event will bubble up to the document
* element.
*/
function pressKey(key, element = document.documentElement) {
const event = new KeyboardEvent('keydown', { key });
element.dispatchEvent(event);
return event;
}
describe('installShortcut', () => { describe('installShortcut', () => {
it('should install a shortcut listener on the document body', () => { it('should install a shortcut listener on the document element', () => {
const onPress = sinon.stub(); const onPress = sinon.stub();
const removeShortcut = installShortcut('a', onPress); const removeShortcut = installShortcut('a', onPress);
const event = new KeyboardEvent('keydown', { key: 'a' });
document.body.dispatchEvent(event); const event = pressKey('a');
removeShortcut(); removeShortcut();
assert.calledWith(onPress, event); assert.calledWith(onPress, event);
...@@ -100,9 +113,8 @@ describe('shared/shortcut', () => { ...@@ -100,9 +113,8 @@ describe('shared/shortcut', () => {
const onPress = sinon.stub(); const onPress = sinon.stub();
const el = document.createElement('div'); const el = document.createElement('div');
const removeShortcut = installShortcut('a', onPress, { rootElement: el }); const removeShortcut = installShortcut('a', onPress, { rootElement: el });
const event = new KeyboardEvent('keydown', { key: 'a' });
el.dispatchEvent(event); const event = pressKey('a', el);
removeShortcut(); removeShortcut();
assert.calledWith(onPress, event); assert.calledWith(onPress, event);
...@@ -111,9 +123,8 @@ describe('shared/shortcut', () => { ...@@ -111,9 +123,8 @@ describe('shared/shortcut', () => {
it('should not trigger if not-matching key is pressed', () => { it('should not trigger if not-matching key is pressed', () => {
const onPress = sinon.stub(); const onPress = sinon.stub();
const removeShortcut = installShortcut('a', onPress); const removeShortcut = installShortcut('a', onPress);
const event = new KeyboardEvent('keydown', { key: 'b' });
document.body.dispatchEvent(event); pressKey('b');
removeShortcut(); removeShortcut();
assert.notCalled(onPress); assert.notCalled(onPress);
...@@ -122,10 +133,9 @@ describe('shared/shortcut', () => { ...@@ -122,10 +133,9 @@ describe('shared/shortcut', () => {
it('should remove shortcut listener when returned callback is called', () => { it('should remove shortcut listener when returned callback is called', () => {
const onPress = sinon.stub(); const onPress = sinon.stub();
const removeShortcut = installShortcut('a', onPress); const removeShortcut = installShortcut('a', onPress);
const event = new KeyboardEvent('keydown', { key: 'a' });
removeShortcut(); removeShortcut();
document.body.dispatchEvent(event); pressKey('a');
assert.notCalled(onPress); assert.notCalled(onPress);
}); });
...@@ -137,11 +147,6 @@ describe('shared/shortcut', () => { ...@@ -137,11 +147,6 @@ describe('shared/shortcut', () => {
return <button onClick={onClick}>Shortcut test</button>; return <button onClick={onClick}>Shortcut test</button>;
} }
function triggerShortcut(keyEventArgs) {
const event = new KeyboardEvent('keydown', keyEventArgs);
document.body.dispatchEvent(event);
}
let container; let container;
beforeEach(() => { beforeEach(() => {
container = document.createElement('div'); container = document.createElement('div');
...@@ -160,7 +165,7 @@ describe('shared/shortcut', () => { ...@@ -160,7 +165,7 @@ describe('shared/shortcut', () => {
act(() => { act(() => {
render(<Button shortcut="a" onClick={onClick} />, container); render(<Button shortcut="a" onClick={onClick} />, container);
}); });
triggerShortcut({ key: 'a' }); pressKey('a');
assert.called(onClick); assert.called(onClick);
}); });
...@@ -171,7 +176,7 @@ describe('shared/shortcut', () => { ...@@ -171,7 +176,7 @@ describe('shared/shortcut', () => {
act(() => { act(() => {
render(<Button onClick={onClick} />, container); render(<Button onClick={onClick} />, container);
}); });
triggerShortcut({ key: 'a' }); pressKey('a');
assert.notCalled(onClick); assert.notCalled(onClick);
}); });
...@@ -183,8 +188,7 @@ describe('shared/shortcut', () => { ...@@ -183,8 +188,7 @@ describe('shared/shortcut', () => {
render(<Button onClick={onClick} />, container); render(<Button onClick={onClick} />, container);
render(null, container); render(null, container);
}); });
pressKey('a');
triggerShortcut({ key: 'a' });
assert.notCalled(onClick); assert.notCalled(onClick);
}); });
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment