From bed0187693873438da6dabc730c7d6d20d3d0138 Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Mon, 17 Feb 2025 09:53:29 +0000 Subject: [PATCH 1/4] refactor dialog-helper to use toggle events --- app/components/primer/dialog_helper.ts | 49 ++++++++++++++------------ package-lock.json | 14 +++++++- package.json | 5 +-- 3 files changed, 42 insertions(+), 26 deletions(-) diff --git a/app/components/primer/dialog_helper.ts b/app/components/primer/dialog_helper.ts index 1382029bd1..0d6ab9c60d 100644 --- a/app/components/primer/dialog_helper.ts +++ b/app/components/primer/dialog_helper.ts @@ -1,3 +1,27 @@ +import 'dialog-toggle-events-polyfill' + +function dialogBeforeToggleHandler(event: Event) { + const {target, newState} = event as ToggleEvent + if (!(target instanceof HTMLDialogElement)) return + if (newState !== 'open') return + const {body} = target.ownerDocument + body.style.setProperty('--dialog-scrollgutter', `${window.innerWidth - body.clientWidth}px`) +} + +function dialogToggleHandler(event: Event) { + const {target, newState} = event as ToggleEvent + if (!(target instanceof HTMLDialogElement)) return + if (newState !== 'open') return + if (target.closest('dialog-helper')?.dialog !== target) return + // We don't want to show the Dialog component as non-modal + if (target.matches('[open]:not(:modal)')) { + // eslint-disable-next-line no-restricted-syntax + target.addEventListener('close', e => e.stopImmediatePropagation(), {once: true}) + target.close() + target.showModal() + } +} + function dialogInvokerButtonHandler(event: Event) { const target = event.target as HTMLElement const button = target?.closest('button') @@ -76,35 +100,14 @@ export class DialogHelperElement extends HTMLElement { const {signal} = (this.#abortController = new AbortController()) document.addEventListener('click', dialogInvokerButtonHandler, true) document.addEventListener('click', this, {signal}) - this.ownerDocument.body.style.setProperty( - '--dialog-scrollgutter', - `${window.innerWidth - this.ownerDocument.body.clientWidth}px`, - ) - new MutationObserver(records => { - for (const record of records) { - if (record.target === this.dialog) { - this.#handleDialogOpenAttribute() - } - } - }).observe(this, {subtree: true, attributeFilter: ['open']}) - this.#handleDialogOpenAttribute() + document.addEventListener('beforetoggle', dialogBeforeToggleHandler, true) + document.addEventListener('toggle', dialogToggleHandler, true) } disconnectedCallback() { this.#abortController?.abort() } - #handleDialogOpenAttribute() { - if (!this.dialog) return - // We don't want to show the Dialog component as non-modal - if (this.dialog.matches('[open]:not(:modal)')) { - // eslint-disable-next-line no-restricted-syntax - this.dialog.addEventListener('close', e => e.stopImmediatePropagation(), {once: true}) - this.dialog.close() - this.dialog.showModal() - } - } - handleEvent(event: MouseEvent) { const target = event.target as HTMLElement const dialog = this.dialog diff --git a/package-lock.json b/package-lock.json index ae85c066c0..9743b22e1c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -21,7 +21,8 @@ "@github/tab-container-element": "^3.1.2", "@oddbird/popover-polyfill": "^0.5.2", "@primer/behaviors": "^1.3.4", - "@primer/live-region-element": "^0.7.1" + "@primer/live-region-element": "^0.7.1", + "dialog-toggle-events-polyfill": "^1.0.1" }, "devDependencies": { "@changesets/changelog-github": "^0.5.0", @@ -4716,6 +4717,12 @@ "node": ">=8" } }, + "node_modules/dialog-toggle-events-polyfill": { + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/dialog-toggle-events-polyfill/-/dialog-toggle-events-polyfill-1.0.1.tgz", + "integrity": "sha512-wS8Ku4Z+BNdDoOEkAow/8c7Ilip/hHmV+75WhRxkWghbM8f909F4+vn+p63SE3Waq9K/nEt0gQrX+zUMVV0Kew==", + "license": "MIT" + }, "node_modules/diff": { "version": "5.2.0", "resolved": "https://registry.npmjs.org/diff/-/diff-5.2.0.tgz", @@ -14986,6 +14993,11 @@ "integrity": "sha512-reYkTUJAZb9gUuZ2RvVCNhVHdg62RHnJ7WJl8ftMi4diZ6NWlciOzQN88pUhSELEwflJht4oQDv0F0BMlwaYtA==", "dev": true }, + "dialog-toggle-events-polyfill": { + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/dialog-toggle-events-polyfill/-/dialog-toggle-events-polyfill-1.0.1.tgz", + "integrity": "sha512-wS8Ku4Z+BNdDoOEkAow/8c7Ilip/hHmV+75WhRxkWghbM8f909F4+vn+p63SE3Waq9K/nEt0gQrX+zUMVV0Kew==" + }, "diff": { "version": "5.2.0", "resolved": "https://registry.npmjs.org/diff/-/diff-5.2.0.tgz", diff --git a/package.json b/package.json index 968567cd0e..a39d71b304 100644 --- a/package.json +++ b/package.json @@ -53,9 +53,10 @@ "@github/relative-time-element": "^4.0.0", "@github/remote-input-element": "^0.4.0", "@github/tab-container-element": "^3.1.2", - "@primer/live-region-element": "^0.7.1", "@oddbird/popover-polyfill": "^0.5.2", - "@primer/behaviors": "^1.3.4" + "@primer/behaviors": "^1.3.4", + "@primer/live-region-element": "^0.7.1", + "dialog-toggle-events-polyfill": "^1.0.1" }, "peerDependencies": { "@primer/primitives": "9.x || 10.x" From 591cfdb24064df90e471445cb88d84c1c33c17f6 Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Mon, 17 Feb 2025 10:02:58 +0000 Subject: [PATCH 2/4] changeset --- .changeset/green-sloths-press.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/green-sloths-press.md diff --git a/.changeset/green-sloths-press.md b/.changeset/green-sloths-press.md new file mode 100644 index 0000000000..7c89cf1e9a --- /dev/null +++ b/.changeset/green-sloths-press.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': patch +--- + +Improve performance of Dialog element From df09d947fd214b5d3aff8bd753044900d0ca6f60 Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Mon, 17 Feb 2025 11:04:52 +0000 Subject: [PATCH 3/4] support dialog initially open --- app/components/primer/dialog_helper.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/app/components/primer/dialog_helper.ts b/app/components/primer/dialog_helper.ts index 0d6ab9c60d..867e17e873 100644 --- a/app/components/primer/dialog_helper.ts +++ b/app/components/primer/dialog_helper.ts @@ -14,11 +14,15 @@ function dialogToggleHandler(event: Event) { if (newState !== 'open') return if (target.closest('dialog-helper')?.dialog !== target) return // We don't want to show the Dialog component as non-modal - if (target.matches('[open]:not(:modal)')) { + forceDialogToOpenAsModal(target) +} + +function forceDialogToOpenAsModal(dialog: HTMLDialogElement) { + if (dialog.matches('[open]:not(:modal)')) { // eslint-disable-next-line no-restricted-syntax - target.addEventListener('close', e => e.stopImmediatePropagation(), {once: true}) - target.close() - target.showModal() + dialog.addEventListener('close', e => e.stopImmediatePropagation(), {once: true}) + dialog.close() + dialog.showModal() } } @@ -102,6 +106,8 @@ export class DialogHelperElement extends HTMLElement { document.addEventListener('click', this, {signal}) document.addEventListener('beforetoggle', dialogBeforeToggleHandler, true) document.addEventListener('toggle', dialogToggleHandler, true) + const {dialog} = this + if (dialog.open) forceDialogToOpenAsModal(dialog) } disconnectedCallback() { From 952e020624ac1361bf8eab2dc97ba7cf6f64a6d8 Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Mon, 17 Feb 2025 11:13:33 +0000 Subject: [PATCH 4/4] ? --- app/components/primer/dialog_helper.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/components/primer/dialog_helper.ts b/app/components/primer/dialog_helper.ts index 867e17e873..a3a24216b4 100644 --- a/app/components/primer/dialog_helper.ts +++ b/app/components/primer/dialog_helper.ts @@ -107,7 +107,7 @@ export class DialogHelperElement extends HTMLElement { document.addEventListener('beforetoggle', dialogBeforeToggleHandler, true) document.addEventListener('toggle', dialogToggleHandler, true) const {dialog} = this - if (dialog.open) forceDialogToOpenAsModal(dialog) + if (dialog?.open) forceDialogToOpenAsModal(dialog) } disconnectedCallback() {