Skip to content

Prototype a version relying on ‘CloseWatcher’ #717

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Prototype a version relying on ‘CloseWatcher’ #717

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
79b1d0f
Prototype a version relying on ‘CloseWatcher’
KittyGiraudel Sep 10, 2024
290eb14
Simplify the close watcher integration a lot
KittyGiraudel Sep 15, 2024
69abe57
Fix the end-to-end tests
KittyGiraudel Sep 16, 2024
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
13 changes: 13 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 11 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,21 @@
},
"./*": "./*"
},
"keywords": ["modal", "dialog", "accessibility", "a11y", "focus"],
"keywords": [
"modal",
"dialog",
"accessibility",
"a11y",
"focus"
],
"author": "Kitty Giraudel (https://kittygiraudel.com)",
"repository": {
"type": "git",
"url": "https://github.com/KittyGiraudel/a11y-dialog"
},
"files": ["dist/*"],
"files": [
"dist/*"
],
"scripts": {
"build": "rollup -c",
"serve": "npx serve cypress/fixtures -p 5000",
Expand All @@ -42,6 +50,7 @@
"@rollup/plugin-node-resolve": "^15.0.1",
"@rollup/plugin-terser": "^0.4.0",
"@rollup/plugin-typescript": "^11.0.0",
"@types/dom-close-watcher": "^1.0.0",
"cypress": "^13.7.1",
"cypress-real-events": "^1.7.6",
"husky": "^9.0.11",
Expand Down
23 changes: 20 additions & 3 deletions src/a11y-dialog.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ export type A11yDialogInstance = InstanceType<typeof A11yDialog>

const SCOPE = 'data-a11y-dialog'

let closeWatcher: CloseWatcher | null = null
Copy link
Owner Author

@KittyGiraudel KittyGiraudel Sep 10, 2024

Choose a reason for hiding this comment

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

First thing I don’t love about the CloseWatcher API is that it’s single-use.

Once the watcher received a close event, it no longer processes anything else. That means we need to instantiate a new watcher every time we open the dialog.

There is also no way to know from an existing instance whether or not it’s been consumed or if it can process a close event. That’s not exposed.

Copy link

@mayank99 mayank99 Sep 13, 2024

Choose a reason for hiding this comment

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

Yeah I agree the API shape is a bit weird. I guess it makes sense that you don't want to constantly keep listening for a close request.

Does this cause an actual problem?


export default class A11yDialog {
private $el: HTMLElement
private id: string
Expand Down Expand Up @@ -48,7 +50,8 @@ export default class A11yDialog {
if (destroyEvent.defaultPrevented) return this

// Hide the dialog to avoid destroying an open instance
this.hide()
if (closeWatcher) closeWatcher.requestClose()
Copy link
Owner Author

@KittyGiraudel KittyGiraudel Sep 10, 2024

Choose a reason for hiding this comment

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

Second thing I find confusing is the difference between .close(..) and .requestClose(..). Honestly not sure I get it.

Copy link

@mayank99 mayank99 Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

From what I understand, the difference is that requestClose() allows the close event to be cancelled (using preventDefault) whereas close() will force close it.

For the main purpose of CloseWatcher (user-initiated close requests), you might be able to get away without using either of these methods.

Edit: It looks like 290eb14 removes the requestClose 👍

else this.hide()

// Remove the click event delegates for our openers and closers
document.removeEventListener('click', this.handleTriggerClicks, true)
Expand All @@ -74,6 +77,16 @@ export default class A11yDialog {
// If the event was prevented, do not continue with the normal behavior
if (showEvent.defaultPrevented) return this

// When opening the dialog, create a new `CloseWatcher` instance, and listen
// for a close event to call our `.hide(..)` method and nuking the close
// watcher once it’s been consumed
if (typeof CloseWatcher !== 'undefined') {
closeWatcher = new CloseWatcher()
closeWatcher.onclose = event => {
this.hide(event)
}
Copy link
Owner Author

@KittyGiraudel KittyGiraudel Sep 10, 2024

Choose a reason for hiding this comment

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

I don’t know whether we should remove the watcher at that point, like this:

Suggested change
closeWatcher.onclose = event => {
this.hide(event)
}
closeWatcher.onclose = event => {
this.hide(event)
closeWatcher = null
}

Copy link

@mayank99 mayank99 Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe it should be defined within local scope to ensure that it automatically gets garbage collected?

Edit: It looks like 290eb14 changes the approach to use a locally scoped CloseWatcher 👍

}

// Keep a reference to the currently focused element to be able to restore
// it later
this.shown = true
Expand Down Expand Up @@ -204,7 +217,10 @@ export default class A11yDialog {
// boundaries
// See: https://github.com/KittyGiraudel/a11y-dialog/issues/712
if (opener) this.show(event)
if (explicitCloser || implicitCloser) this.hide(event)
if (explicitCloser || implicitCloser) {
if (closeWatcher) closeWatcher.requestClose()
else this.hide(event)
}
Copy link
Owner Author

@KittyGiraudel KittyGiraudel Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is a breaking change, because we no longer can provide access to the “closer” (as in the element that was interacted with to close the dialog) via the close watcher. That’s because we can’t pass an event to the .close(..) or .requestClose(..) method which would be forwarded to the onclose callback. So this is a bit shitty and may be a deal breaker for this integration.

Screenshot 2024-09-10 at 11 35 57

}

/**
Expand Down Expand Up @@ -242,7 +258,8 @@ export default class A11yDialog {
!hasOpenPopover
) {
event.preventDefault()
this.hide(event)
if (closeWatcher) closeWatcher.requestClose()
else this.hide(event)
}

// If the dialog is shown and the TAB key is pressed, make sure the focus
Expand Down