From 2acab3d8a358e29d44267a91211df2906746cc2e Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Wed, 11 Aug 2021 23:51:55 +0200 Subject: [PATCH 01/19] Add focus trap util --- js/package.json | 1 + js/src/common/compat.js | 2 ++ js/src/common/utils/focusTrap.ts | 29 +++++++++++++++++++++++++++++ js/yarn.lock | 17 +++++++++++++++++ 4 files changed, 49 insertions(+) create mode 100644 js/src/common/utils/focusTrap.ts diff --git a/js/package.json b/js/package.json index 0fa66d1f9b..c311d4f62b 100644 --- a/js/package.json +++ b/js/package.json @@ -9,6 +9,7 @@ "clsx": "^1.1.1", "color-thief-browser": "^2.0.2", "dayjs": "^1.10.7", + "focus-trap": "^6.7.1", "jquery": "^3.6.0", "jquery.hotkeys": "^0.1.0", "mithril": "^2.0.4", diff --git a/js/src/common/compat.js b/js/src/common/compat.js index d17bd84cc5..9111a642a9 100644 --- a/js/src/common/compat.js +++ b/js/src/common/compat.js @@ -31,6 +31,7 @@ import extractText from './utils/extractText'; import formatNumber from './utils/formatNumber'; import mapRoutes from './utils/mapRoutes'; import withAttr from './utils/withAttr'; +import * as FocusTrap from './utils/focusTrap'; import Notification from './models/Notification'; import User from './models/User'; import Post from './models/Post'; @@ -116,6 +117,7 @@ export default { 'utils/withAttr': withAttr, 'utils/throttleDebounce': ThrottleDebounce, 'utils/isObject': isObject, + 'utils/focusTrap': FocusTrap, 'models/Notification': Notification, 'models/User': User, 'models/Post': Post, diff --git a/js/src/common/utils/focusTrap.ts b/js/src/common/utils/focusTrap.ts new file mode 100644 index 0000000000..3724aea9a1 --- /dev/null +++ b/js/src/common/utils/focusTrap.ts @@ -0,0 +1,29 @@ +import { createFocusTrap as _createFocusTrap } from 'focus-trap'; + +/** + * Creates a focus trap for the given element with the given options. + * + * This function applies some default options that are different to the library. + * Your own options still override these custom defaults: + * + * ```json + * { + escapeDeactivates: false, + * } + * ``` + * + * @param element The element to be the focus trap, or a selector that will be used to find the element. + * + * @see https://github.com/focus-trap/focus-trap#readme - Library documentation + */ +function createFocusTrap(...args: Parameters): ReturnType { + args[1] = { + escapeDeactivates: false, + ...args[1], + }; + + return _createFocusTrap(...args); +} + +export * from 'focus-trap'; +export { createFocusTrap }; diff --git a/js/yarn.lock b/js/yarn.lock index 6ba8222f7d..ee08d71c40 100644 --- a/js/yarn.lock +++ b/js/yarn.lock @@ -1404,6 +1404,7 @@ __metadata: expose-loader: ^3.1.0 flarum-tsconfig: ^1.0.2 flarum-webpack-config: ^2.0.0 + focus-trap: ^6.7.1 jquery: ^3.6.0 jquery.hotkeys: ^0.1.0 mithril: ^2.0.4 @@ -2565,6 +2566,15 @@ __metadata: languageName: node linkType: hard +"focus-trap@npm:^6.7.1": + version: 6.7.1 + resolution: "focus-trap@npm:6.7.1" + dependencies: + tabbable: ^5.2.1 + checksum: b96c54a6a2976f8509ed8447ce3a8b76db3801b9c170f278f60b0c878478f2bb2ebc6dbe3ccd7157006b9a7ad9a86c18283efff0f3e387e29ba3ea89d8687b9c + languageName: node + linkType: hard + "follow-redirects@npm:^1.14.0": version: 1.14.5 resolution: "follow-redirects@npm:1.14.5" @@ -3896,6 +3906,13 @@ __metadata: languageName: node linkType: hard +"tabbable@npm:^5.2.1": + version: 5.2.1 + resolution: "tabbable@npm:5.2.1" + checksum: d26e9eeb880c4c78b59244bac2c931ad46f6c64a01e5c15ba6da348dc86442222912733846a9e63373342a81fa15d4afb31267606b38431510d10b0fb9ec9bba + languageName: node + linkType: hard + "tapable@npm:^2.1.1, tapable@npm:^2.2.0": version: 2.2.1 resolution: "tapable@npm:2.2.1" From 08f4bdb033125b3c654d378504970d1cf0a735e4 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Thu, 12 Aug 2021 00:17:26 +0200 Subject: [PATCH 02/19] Add focus trap to Modals Fixes #2663 --- js/src/common/components/ModalManager.tsx | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/js/src/common/components/ModalManager.tsx b/js/src/common/components/ModalManager.tsx index a9d55a1243..7c73a5095a 100644 --- a/js/src/common/components/ModalManager.tsx +++ b/js/src/common/components/ModalManager.tsx @@ -1,4 +1,5 @@ import Component from '../Component'; +import { createFocusTrap, FocusTrap } from '../utils/focusTrap'; import type Mithril from 'mithril'; import type ModalManagerState from '../states/ModalManagerState'; @@ -13,7 +14,9 @@ interface IModalManagerAttrs { * overwrite the previous one. */ export default class ModalManager extends Component { - view() { + focusTrap: FocusTrap | undefined; + + view(vnode: Mithril.VnodeDOM): Mithril.Children { const modal = this.attrs.state.modal; return ( @@ -29,13 +32,19 @@ export default class ModalManager extends Component { ); } - oncreate(vnode: Mithril.VnodeDOM) { + oncreate(vnode: Mithril.VnodeDOM): void { super.oncreate(vnode); // Ensure the modal state is notified about a closed modal, even when the // DOM-based Bootstrap JavaScript code triggered the closing of the modal, // e.g. via ESC key or a click on the modal backdrop. this.$().on('hidden.bs.modal', this.attrs.state.close.bind(this.attrs.state)); + + this.focusTrap = createFocusTrap(this.element as HTMLElement); + } + + onupdate(vnode: Mithril.VnodeDOM): void { + super.onupdate(vnode); } animateShow(readyCallback: () => void): void { @@ -59,10 +68,14 @@ export default class ModalManager extends Component { keyboard: dismissible, }) .modal('show'); + + if (this.focusTrap) this.focusTrap.activate(); } animateHide(): void { // @ts-expect-error: No typings available for Bootstrap modals. this.$().modal('hide'); + + if (this.focusTrap) this.focusTrap.deactivate(); } } From c941aa70612cd43cdb5aae2e15cfa98d835ff462 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Thu, 12 Aug 2021 11:46:18 +0200 Subject: [PATCH 03/19] Split tab press into `onTab` handler --- js/src/forum/utils/KeyboardNavigatable.ts | 25 ++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/js/src/forum/utils/KeyboardNavigatable.ts b/js/src/forum/utils/KeyboardNavigatable.ts index 53b1b2d596..ff64f99545 100644 --- a/js/src/forum/utils/KeyboardNavigatable.ts +++ b/js/src/forum/utils/KeyboardNavigatable.ts @@ -51,7 +51,7 @@ export default class KeyboardNavigatable { /** * Provide a callback to be executed when the current item is selected.. * - * This will be triggered by the Return and Tab keys.. + * This will be triggered by the Return key. */ onSelect(callback: KeyboardEventHandler): KeyboardNavigatable { const handler: KeyboardEventHandler = (e) => { @@ -59,12 +59,27 @@ export default class KeyboardNavigatable { callback(e); }; - this.callbacks.set(9, handler); this.callbacks.set(13, handler); return this; } + /** + * Provide a callback to be executed when the current item is tabbed into. + * + * This will be triggered by the Tab key. + */ + onTab(callback: KeyboardEventHandler): KeyboardNavigatable { + const handler: KeyboardEventHandler = (e) => { + e.preventDefault(); + callback(e); + }; + + this.callbacks.set(9, handler); + + return this; + } + /** * Provide a callback to be executed when the navigation is canceled. * @@ -84,10 +99,6 @@ export default class KeyboardNavigatable { * Provide a callback to be executed when previous input is removed. * * This will be triggered by the Backspace key. - * - * @public - * @param {KeyboardNavigatable~keyCallback} callback - * @return {KeyboardNavigatable} */ onRemove(callback: KeyboardEventHandler): KeyboardNavigatable { this.callbacks.set(8, (e) => { @@ -112,7 +123,7 @@ export default class KeyboardNavigatable { /** * Set up the navigation key bindings on the given jQuery element. */ - bindTo($element: JQuery) { + bindTo($element: JQuery) { // Handle navigation key events on the navigatable element. $element[0].addEventListener('keydown', this.navigate.bind(this)); } From 9afccf89600a0ad7ccb23b53a389e1c685bf63b2 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Thu, 12 Aug 2021 11:58:26 +0200 Subject: [PATCH 04/19] Remove deprecated code --- js/src/common/utils/Drawer.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/js/src/common/utils/Drawer.js b/js/src/common/utils/Drawer.js index d0bfa7da6f..2184df79d5 100644 --- a/js/src/common/utils/Drawer.js +++ b/js/src/common/utils/Drawer.js @@ -7,7 +7,7 @@ export default class Drawer { constructor() { // Set up an event handler so that whenever the content area is tapped, // the drawer will close. - $('#content').click((e) => { + $('#content').on('click', (e) => { if (this.isOpen()) { e.preventDefault(); this.hide(); @@ -61,10 +61,7 @@ export default class Drawer { show() { $('#app').addClass('drawerOpen'); - this.$backdrop = $('
') - .addClass('drawer-backdrop fade') - .appendTo('body') - .click(() => this.hide()); + this.$backdrop = $('
').addClass('drawer-backdrop fade').appendTo('body').on('click', this.hide.bind(this)); setTimeout(() => this.$backdrop.addClass('in')); } From c50420a014a8480b7956e0fb8099876b2a560772 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Thu, 12 Aug 2021 12:08:40 +0200 Subject: [PATCH 05/19] Use requestAnimationFrame instead of setTimeout --- js/src/common/utils/Drawer.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/js/src/common/utils/Drawer.js b/js/src/common/utils/Drawer.js index 2184df79d5..72b15fd9fa 100644 --- a/js/src/common/utils/Drawer.js +++ b/js/src/common/utils/Drawer.js @@ -63,6 +63,8 @@ export default class Drawer { this.$backdrop = $('
').addClass('drawer-backdrop fade').appendTo('body').on('click', this.hide.bind(this)); - setTimeout(() => this.$backdrop.addClass('in')); + requestAnimationFrame(() => { + this.$backdrop.addClass('in'); + }); } } From 2b46243761d07bfafeba39a31554076f4051f3a2 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Thu, 12 Aug 2021 12:19:20 +0200 Subject: [PATCH 06/19] Reduce code duplication --- js/src/common/utils/Drawer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/common/utils/Drawer.js b/js/src/common/utils/Drawer.js index 72b15fd9fa..be7f9ac5eb 100644 --- a/js/src/common/utils/Drawer.js +++ b/js/src/common/utils/Drawer.js @@ -41,7 +41,7 @@ export default class Drawer { const $app = $('#app'); - if (!$app.hasClass('drawerOpen')) return; + if (!this.isOpen()) return; const $drawer = $('#drawer'); From 84ab9223cc1e441e885a98ac4ab2cb142189fd2d Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Thu, 12 Aug 2021 12:20:19 +0200 Subject: [PATCH 07/19] Implement focus trap in nav drawer Fixes #2665 --- js/src/common/utils/Drawer.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/js/src/common/utils/Drawer.js b/js/src/common/utils/Drawer.js index be7f9ac5eb..0ccf838881 100644 --- a/js/src/common/utils/Drawer.js +++ b/js/src/common/utils/Drawer.js @@ -1,9 +1,16 @@ +import { createFocusTrap } from './focusTrap'; + /** * The `Drawer` class controls the page's drawer. The drawer is the area the * slides out from the left on mobile devices; it contains the header and the * footer. */ export default class Drawer { + /** + * @type {import('./focusTrap').FocusTrap} + */ + focusTrap; + constructor() { // Set up an event handler so that whenever the content area is tapped, // the drawer will close. @@ -13,6 +20,8 @@ export default class Drawer { this.hide(); } }); + + this.focusTrap = createFocusTrap('#drawer', { allowOutsideClick: true }); } /** @@ -41,6 +50,8 @@ export default class Drawer { const $app = $('#app'); + this.focusTrap.deactivate(); + if (!this.isOpen()) return; const $drawer = $('#drawer'); @@ -65,6 +76,8 @@ export default class Drawer { requestAnimationFrame(() => { this.$backdrop.addClass('in'); + + this.focusTrap.activate(); }); } } From b32059f46db85d0b468ef393feb128f7aaa2f7c4 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Thu, 12 Aug 2021 13:19:13 +0200 Subject: [PATCH 08/19] Hide drawer when window is resized to be bigger Fixes issue where focus trap would remain on the drawer when it is just the app header, if the drawer was opened then the window was made larger. --- js/src/common/utils/Drawer.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/js/src/common/utils/Drawer.js b/js/src/common/utils/Drawer.js index 0ccf838881..d0c2c42687 100644 --- a/js/src/common/utils/Drawer.js +++ b/js/src/common/utils/Drawer.js @@ -1,4 +1,5 @@ import { createFocusTrap } from './focusTrap'; +import { throttle } from './throttleDebounce'; /** * The `Drawer` class controls the page's drawer. The drawer is the area the @@ -24,6 +25,17 @@ export default class Drawer { this.focusTrap = createFocusTrap('#drawer', { allowOutsideClick: true }); } + resizeHandler(e) { + if (this.isOpen()) { + if (document.documentElement.style.getPropertyValue('--flarum-screen') !== 'phone') { + // Drawer is open but we've made window bigger, so hide it. + this.hide(); + } + } + } + + throttledResizeHandler = throttle(500, this.resizeHandler.bind(this)); + /** * Check whether or not the drawer is currently open. * @@ -51,6 +63,7 @@ export default class Drawer { const $app = $('#app'); this.focusTrap.deactivate(); + window.removeEventListener('resize', this.throttledResizeHandler); if (!this.isOpen()) return; @@ -72,6 +85,8 @@ export default class Drawer { show() { $('#app').addClass('drawerOpen'); + window.addEventListener('resize', this.throttledResizeHandler, { passive: true }); + this.$backdrop = $('
').addClass('drawer-backdrop fade').appendTo('body').on('click', this.hide.bind(this)); requestAnimationFrame(() => { From 836ad74388d097ee6e853ce3dd4d1fa5b3670705 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Thu, 12 Aug 2021 13:24:04 +0200 Subject: [PATCH 09/19] Simplify conditional function calls --- js/src/common/components/ModalManager.tsx | 4 ++-- js/src/common/utils/Drawer.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/js/src/common/components/ModalManager.tsx b/js/src/common/components/ModalManager.tsx index 7c73a5095a..68ff6fc74e 100644 --- a/js/src/common/components/ModalManager.tsx +++ b/js/src/common/components/ModalManager.tsx @@ -69,13 +69,13 @@ export default class ModalManager extends Component { }) .modal('show'); - if (this.focusTrap) this.focusTrap.activate(); + this.focusTrap.activate?.(); } animateHide(): void { // @ts-expect-error: No typings available for Bootstrap modals. this.$().modal('hide'); - if (this.focusTrap) this.focusTrap.deactivate(); + this.focusTrap.deactivate?.(); } } diff --git a/js/src/common/utils/Drawer.js b/js/src/common/utils/Drawer.js index d0c2c42687..779f3b1926 100644 --- a/js/src/common/utils/Drawer.js +++ b/js/src/common/utils/Drawer.js @@ -74,7 +74,7 @@ export default class Drawer { $app.removeClass('drawerOpen'); - if (this.$backdrop) this.$backdrop.remove(); + this.$backdrop.remove?.(); } /** From 849f9a0f6f5f6e57cfe33bf69d2d5c91d6dbabcc Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Thu, 12 Aug 2021 14:34:04 +0200 Subject: [PATCH 10/19] Fix modal focus trap --- js/src/common/components/ModalManager.tsx | 28 +++++++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/js/src/common/components/ModalManager.tsx b/js/src/common/components/ModalManager.tsx index 68ff6fc74e..7458bc801b 100644 --- a/js/src/common/components/ModalManager.tsx +++ b/js/src/common/components/ModalManager.tsx @@ -1,8 +1,12 @@ import Component from '../Component'; + import { createFocusTrap, FocusTrap } from '../utils/focusTrap'; +import { tabbable } from 'tabbable'; -import type Mithril from 'mithril'; import type ModalManagerState from '../states/ModalManagerState'; +import type Mithril from 'mithril'; + +window.tabbable = tabbable; interface IModalManagerAttrs { state: ModalManagerState; @@ -14,7 +18,12 @@ interface IModalManagerAttrs { * overwrite the previous one. */ export default class ModalManager extends Component { - focusTrap: FocusTrap | undefined; + protected focusTrap: FocusTrap | undefined; + + /** + * Whether a modal is currently shown by this modal manager. + */ + protected modalShown: boolean = false; view(vnode: Mithril.VnodeDOM): Mithril.Children { const modal = this.attrs.state.modal; @@ -45,6 +54,15 @@ export default class ModalManager extends Component { onupdate(vnode: Mithril.VnodeDOM): void { super.onupdate(vnode); + + requestAnimationFrame(() => { + try { + if (this.modalShown) this.focusTrap!.activate?.(); + else this.focusTrap!.deactivate?.(); + } catch { + // We can expect errors to occur here due to the nature of mithril rendering + } + }); } animateShow(readyCallback: () => void): void { @@ -52,6 +70,8 @@ export default class ModalManager extends Component { const dismissible = !!this.attrs.state.modal.componentClass.isDismissible; + this.modalShown = true; + // If we are opening this modal while another modal is already open, // the shown event will not run, because the modal is already open. // So, we need to manually trigger the readyCallback. @@ -68,14 +88,12 @@ export default class ModalManager extends Component { keyboard: dismissible, }) .modal('show'); - - this.focusTrap.activate?.(); } animateHide(): void { // @ts-expect-error: No typings available for Bootstrap modals. this.$().modal('hide'); - this.focusTrap.deactivate?.(); + this.modalShown = false; } } From bdc33355a4e893ae4c8218eeac6f4da0be127d77 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Sat, 14 Aug 2021 22:20:23 +0200 Subject: [PATCH 11/19] Remove debug code --- js/src/common/components/ModalManager.tsx | 3 --- 1 file changed, 3 deletions(-) diff --git a/js/src/common/components/ModalManager.tsx b/js/src/common/components/ModalManager.tsx index 7458bc801b..d46899eec2 100644 --- a/js/src/common/components/ModalManager.tsx +++ b/js/src/common/components/ModalManager.tsx @@ -1,13 +1,10 @@ import Component from '../Component'; import { createFocusTrap, FocusTrap } from '../utils/focusTrap'; -import { tabbable } from 'tabbable'; import type ModalManagerState from '../states/ModalManagerState'; import type Mithril from 'mithril'; -window.tabbable = tabbable; - interface IModalManagerAttrs { state: ModalManagerState; } From b07f676d08d8eb6d71c679830858b8f195bf398c Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Sat, 14 Aug 2021 22:34:32 +0200 Subject: [PATCH 12/19] Simplify resize handler conditional statements --- js/src/common/utils/Drawer.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/js/src/common/utils/Drawer.js b/js/src/common/utils/Drawer.js index 779f3b1926..6f0aba19e3 100644 --- a/js/src/common/utils/Drawer.js +++ b/js/src/common/utils/Drawer.js @@ -26,11 +26,9 @@ export default class Drawer { } resizeHandler(e) { - if (this.isOpen()) { - if (document.documentElement.style.getPropertyValue('--flarum-screen') !== 'phone') { - // Drawer is open but we've made window bigger, so hide it. - this.hide(); - } + if (this.isOpen() && app.screen() !== 'phone') { + // Drawer is open but we've made window bigger, so hide it. + this.hide(); } } From c92b988b4e6b7106235246f4154880f500df4fe4 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Sat, 14 Aug 2021 22:34:48 +0200 Subject: [PATCH 13/19] Add info about reasoning of resize handler --- js/src/common/utils/Drawer.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/js/src/common/utils/Drawer.js b/js/src/common/utils/Drawer.js index 6f0aba19e3..138a375a26 100644 --- a/js/src/common/utils/Drawer.js +++ b/js/src/common/utils/Drawer.js @@ -25,6 +25,16 @@ export default class Drawer { this.focusTrap = createFocusTrap('#drawer', { allowOutsideClick: true }); } + /** + * Handler for the `resize` event on `window`. + * + * This is used to close the drawer when the viewport is widened past the `phone` size. + * At this point, the drawer turns into the standard header that we see on desktop, but + * the drawer is still registered as 'open' internally. + * + * This causes issues with the focus trap, resulting in focus becoming trapped within + * the header on desktop viewports. + */ resizeHandler(e) { if (this.isOpen() && app.screen() !== 'phone') { // Drawer is open but we've made window bigger, so hide it. From 0fca8cd80d5805c019bad934e00b80b38d0092bf Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Sat, 14 Aug 2021 23:08:51 +0200 Subject: [PATCH 14/19] Prefer native JS methods over jQuery --- js/src/common/utils/Drawer.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/js/src/common/utils/Drawer.js b/js/src/common/utils/Drawer.js index 138a375a26..cc284d44f7 100644 --- a/js/src/common/utils/Drawer.js +++ b/js/src/common/utils/Drawer.js @@ -12,16 +12,23 @@ export default class Drawer { */ focusTrap; + /** + * @type {HTMLElement} + */ + appElement; + constructor() { // Set up an event handler so that whenever the content area is tapped, // the drawer will close. - $('#content').on('click', (e) => { + document.getElementById('content').addEventListener('click', (e) => { if (this.isOpen()) { e.preventDefault(); this.hide(); } }); + this.appElement = document.getElementById('app'); + this.focusTrap = createFocusTrap('#drawer', { allowOutsideClick: true }); } @@ -68,8 +75,6 @@ export default class Drawer { * More info: https://github.com/flarum/core/pull/2666#discussion_r595381014 */ - const $app = $('#app'); - this.focusTrap.deactivate(); window.removeEventListener('resize', this.throttledResizeHandler); @@ -80,7 +85,7 @@ export default class Drawer { // Used to prevent `visibility: hidden` from breaking the exit animation $drawer.css('visibility', 'visible').one('transitionend', () => $drawer.css('visibility', '')); - $app.removeClass('drawerOpen'); + this.appElement.classList.remove('drawerOpen'); this.$backdrop.remove?.(); } @@ -91,7 +96,7 @@ export default class Drawer { * @public */ show() { - $('#app').addClass('drawerOpen'); + this.appElement.classList.add('drawerOpen'); window.addEventListener('resize', this.throttledResizeHandler, { passive: true }); From 1f5b0bbe364583be095a6c2c6469a85711125ed1 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Sat, 14 Aug 2021 23:19:41 +0200 Subject: [PATCH 15/19] Update conditional function call to handle `undefined` --- js/src/common/utils/Drawer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/common/utils/Drawer.js b/js/src/common/utils/Drawer.js index cc284d44f7..0d5435e40b 100644 --- a/js/src/common/utils/Drawer.js +++ b/js/src/common/utils/Drawer.js @@ -87,7 +87,7 @@ export default class Drawer { this.appElement.classList.remove('drawerOpen'); - this.$backdrop.remove?.(); + this.$backdrop?.remove?.(); } /** From 9b01ad4774334368dcf0063a99d169594567f4f3 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Mon, 16 Aug 2021 23:43:44 +0100 Subject: [PATCH 16/19] Expose screen sizes as CSS custom properties --- less/common/root.less | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/less/common/root.less b/less/common/root.less index 2790ef75b1..1b0e6988ad 100644 --- a/less/common/root.less +++ b/less/common/root.less @@ -105,6 +105,13 @@ // available to the JS code. --flarum-screen: none; + --screen-phone-max: @screen-phone-max; + --screen-tablet: @screen-tablet; + --screen-tablet-max: @screen-tablet-max; + --screen-desktop: @screen-desktop; + --screen-desktop-max: @screen-desktop-max; + --screen-desktop-hd: @screen-desktop-hd; + @media @phone { --flarum-screen: phone; } @media @tablet { --flarum-screen: tablet; } @media @desktop { --flarum-screen: desktop; } From 41f4c145feda750b070a1b6d66eb5c2e9d5cf33c Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Fri, 3 Sep 2021 17:38:42 +0100 Subject: [PATCH 17/19] Use `window.matchMedia` rather than resize handler --- js/src/common/utils/Drawer.js | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/js/src/common/utils/Drawer.js b/js/src/common/utils/Drawer.js index 0d5435e40b..6fa2d546e0 100644 --- a/js/src/common/utils/Drawer.js +++ b/js/src/common/utils/Drawer.js @@ -1,5 +1,4 @@ import { createFocusTrap } from './focusTrap'; -import { throttle } from './throttleDebounce'; /** * The `Drawer` class controls the page's drawer. The drawer is the area the @@ -13,7 +12,7 @@ export default class Drawer { focusTrap; /** - * @type {HTMLElement} + * @type {HTMLDivElement} */ appElement; @@ -28,8 +27,10 @@ export default class Drawer { }); this.appElement = document.getElementById('app'); - this.focusTrap = createFocusTrap('#drawer', { allowOutsideClick: true }); + this.drawerAailableMediaQuery = window.matchMedia( + `(max-width: ${getComputedStyle(document.documentElement).getPropertyValue('--screen-phone-max')})` + ); } /** @@ -41,24 +42,31 @@ export default class Drawer { * * This causes issues with the focus trap, resulting in focus becoming trapped within * the header on desktop viewports. + * + * @internal */ - resizeHandler(e) { - if (this.isOpen() && app.screen() !== 'phone') { + resizeHandler = ((e) => { + console.log(this, e); + if (!e.matches && this.isOpen()) { // Drawer is open but we've made window bigger, so hide it. this.hide(); } - } + }).bind(this); - throttledResizeHandler = throttle(500, this.resizeHandler.bind(this)); + /** + * @internal + * @type {MediaQueryList} + */ + drawerAailableMediaQuery; /** * Check whether or not the drawer is currently open. * - * @return {Boolean} + * @return {boolean} * @public */ isOpen() { - return $('#app').hasClass('drawerOpen'); + return this.appElement.classList.contains('drawerOpen'); } /** @@ -76,7 +84,7 @@ export default class Drawer { */ this.focusTrap.deactivate(); - window.removeEventListener('resize', this.throttledResizeHandler); + this.drawerAailableMediaQuery.removeListener(this.resizeHandler); if (!this.isOpen()) return; @@ -98,7 +106,7 @@ export default class Drawer { show() { this.appElement.classList.add('drawerOpen'); - window.addEventListener('resize', this.throttledResizeHandler, { passive: true }); + this.drawerAailableMediaQuery.addListener(this.resizeHandler); this.$backdrop = $('
').addClass('drawer-backdrop fade').appendTo('body').on('click', this.hide.bind(this)); From 33d65ba607ff86723c1d4f0ea9d52d8eceeed9fd Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Fri, 10 Sep 2021 22:39:57 +0100 Subject: [PATCH 18/19] Fix spelling error Co-authored-by: David Sevilla Martin --- js/src/common/utils/Drawer.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/js/src/common/utils/Drawer.js b/js/src/common/utils/Drawer.js index 6fa2d546e0..c0003dd1ea 100644 --- a/js/src/common/utils/Drawer.js +++ b/js/src/common/utils/Drawer.js @@ -28,7 +28,7 @@ export default class Drawer { this.appElement = document.getElementById('app'); this.focusTrap = createFocusTrap('#drawer', { allowOutsideClick: true }); - this.drawerAailableMediaQuery = window.matchMedia( + this.drawerAvailableMediaQuery = window.matchMedia( `(max-width: ${getComputedStyle(document.documentElement).getPropertyValue('--screen-phone-max')})` ); } @@ -57,7 +57,7 @@ export default class Drawer { * @internal * @type {MediaQueryList} */ - drawerAailableMediaQuery; + drawerAvailableMediaQuery; /** * Check whether or not the drawer is currently open. @@ -84,7 +84,7 @@ export default class Drawer { */ this.focusTrap.deactivate(); - this.drawerAailableMediaQuery.removeListener(this.resizeHandler); + this.drawerAvailableMediaQuery.removeListener(this.resizeHandler); if (!this.isOpen()) return; @@ -106,7 +106,7 @@ export default class Drawer { show() { this.appElement.classList.add('drawerOpen'); - this.drawerAailableMediaQuery.addListener(this.resizeHandler); + this.drawerAvailableMediaQuery.addListener(this.resizeHandler); this.$backdrop = $('
').addClass('drawer-backdrop fade').appendTo('body').on('click', this.hide.bind(this)); From 23a03eb6c88f9121347fa7fddf3f8ecd448d950c Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Fri, 10 Sep 2021 22:53:25 +0100 Subject: [PATCH 19/19] Remove breaking change --- js/src/forum/components/Search.tsx | 2 +- js/src/forum/utils/KeyboardNavigatable.ts | 27 +++++++++++++++++------ 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/js/src/forum/components/Search.tsx b/js/src/forum/components/Search.tsx index bf45be7486..169197d137 100644 --- a/js/src/forum/components/Search.tsx +++ b/js/src/forum/components/Search.tsx @@ -202,7 +202,7 @@ export default class Search extends Compone this.navigator .onUp(() => this.setIndex(this.getCurrentNumericIndex() - 1, true)) .onDown(() => this.setIndex(this.getCurrentNumericIndex() + 1, true)) - .onSelect(this.selectResult.bind(this)) + .onSelect(this.selectResult.bind(this), true) .onCancel(this.clear.bind(this)) .bindTo($input); diff --git a/js/src/forum/utils/KeyboardNavigatable.ts b/js/src/forum/utils/KeyboardNavigatable.ts index ff64f99545..667ac10b50 100644 --- a/js/src/forum/utils/KeyboardNavigatable.ts +++ b/js/src/forum/utils/KeyboardNavigatable.ts @@ -1,6 +1,18 @@ type KeyboardEventHandler = (event: KeyboardEvent) => void; type ShouldHandle = (event: KeyboardEvent) => boolean; +enum Keys { + Enter = 13, + Escape = 27, + Space = 32, + ArrowUp = 38, + ArrowDown = 40, + ArrowLeft = 37, + ArrowRight = 39, + Tab = 9, + Backspace = 8, +} + /** * The `KeyboardNavigatable` class manages lists that can be navigated with the * keyboard, calling callbacks for each actions. @@ -26,7 +38,7 @@ export default class KeyboardNavigatable { * This will be triggered by the Up key. */ onUp(callback: KeyboardEventHandler): KeyboardNavigatable { - this.callbacks.set(38, (e) => { + this.callbacks.set(Keys.ArrowUp, (e) => { e.preventDefault(); callback(e); }); @@ -40,7 +52,7 @@ export default class KeyboardNavigatable { * This will be triggered by the Down key. */ onDown(callback: KeyboardEventHandler): KeyboardNavigatable { - this.callbacks.set(40, (e) => { + this.callbacks.set(Keys.ArrowDown, (e) => { e.preventDefault(); callback(e); }); @@ -51,15 +63,16 @@ export default class KeyboardNavigatable { /** * Provide a callback to be executed when the current item is selected.. * - * This will be triggered by the Return key. + * This will be triggered by the Return key (and Tab key, if not disabled). */ - onSelect(callback: KeyboardEventHandler): KeyboardNavigatable { + onSelect(callback: KeyboardEventHandler, ignoreTabPress: boolean = false): KeyboardNavigatable { const handler: KeyboardEventHandler = (e) => { e.preventDefault(); callback(e); }; - this.callbacks.set(13, handler); + if (!ignoreTabPress) this.callbacks.set(Keys.Tab, handler); + this.callbacks.set(Keys.Enter, handler); return this; } @@ -86,7 +99,7 @@ export default class KeyboardNavigatable { * This will be triggered by the Escape key. */ onCancel(callback: KeyboardEventHandler): KeyboardNavigatable { - this.callbacks.set(27, (e) => { + this.callbacks.set(Keys.Escape, (e) => { e.stopPropagation(); e.preventDefault(); callback(e); @@ -101,7 +114,7 @@ export default class KeyboardNavigatable { * This will be triggered by the Backspace key. */ onRemove(callback: KeyboardEventHandler): KeyboardNavigatable { - this.callbacks.set(8, (e) => { + this.callbacks.set(Keys.Backspace, (e) => { if (e instanceof KeyboardEvent && e.target instanceof HTMLInputElement && e.target.selectionStart === 0 && e.target.selectionEnd === 0) { callback(e); e.preventDefault();