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

Add custom event in escape handler #695

Conversation

joe-watkins
Copy link

Hello! ;)

This PR is for #694 It adds a custom event to the Escape key handler.

The name of this custom event is hardcoded at this stage and is called dialog:escape

Use Case: Dialog has a child component that needs the escape key for its own functionality. For example, an ARIA Combobox with an expanded Listbox where escape should collapse that expanded Listbox instead of closing the dialog.

If a11y-dialog users wanted to get in front of the default Escape key functionality they would listen for this new custom event dialog:escape and simply use preventDefault(); in that component's JavaScript.

Example use: ARIA APG Select Combobox init

Select.prototype.init = function () {
    /* ... */
    document.addEventListener('dialog:escape', (event) => {
        if (this.open) { // Check if the dropdown is open
            this.updateMenuState(false); // Close the dropdown
            event.preventDefault(); // Prevent default behavior stops escape from working in a11y-dialog
        }
    });
}

I look forward to seeing what you cook up for documentation and tests and hope others enjoy this addition. Thanks for supporting a11y-dialog too @KittyGiraudel

)
const customEvent = new CustomEvent(type, {
detail: event,
bubbles: true,
Copy link
Owner

Choose a reason for hiding this comment

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

I am unclear whether making all events bubbling can have unexpected side-effects, but I think it should be fine?

@KittyGiraudel
Copy link
Owner

I spent a bit of time on this:

  1. I massaged the code a little bit to use the fire method (which already creates and dispatches CustomEvent). One thing I’m not super sure about is whether it’s fine to make all dialog events bubble, but I think it should be safe?
  2. I added some end-to-end tests to make sure that this feature works as expected.

I think from a code perspective, it looks pretty good now. But it would be nice to have a review again @joe-watkins. :)

@KittyGiraudel
Copy link
Owner

KittyGiraudel commented Aug 9, 2024

Actually before moving on, I want to have a look whether we could simplify this whole thing by always checking whether the custom event was cancelled, and if it was, not do anything (show, hide, destroy…). This way, we could probably do something like:

Select.prototype.init = function () {
    /* ... */
    document.addEventListener('hide', (event) => {
        const closingWithEscape = event.detail?.key === 'Escape';
        if (this.open && closingWithEscape) { // Check if the dropdown is open
            this.updateMenuState(false); // Close the dropdown
            event.preventDefault(); // Prevent default behavior stops escape from working in a11y-dialog
        }
    });
}

That would make it better I think. But I need to see if I can make it work.

@KittyGiraudel
Copy link
Owner

Moved to #696. Wanna review it @joe-watkins? :)

@KittyGiraudel
Copy link
Owner

Closed in favor of #696.

@joe-watkins
Copy link
Author

@KittyGiraudel Thanks so much for thinking this through more and coming up with a more sound solution :) I appreciate the effort!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants