fix: Check for :modal selector support before using it#33
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a feature detection check for the :modal CSS pseudo-class selector to prevent syntax errors in environments like JSDOM (used by Jest) that don't support this selector.
Changes:
- Added CSS feature detection using
CSS.supports("selector(:modal)")before using the:modalselector - Fallback to
dialog[open]selector when:modalis not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // This can be checked by seeing if the element is within the modal, if the modal is present. | ||
| (this.element.ownerDocument | ||
| .querySelector(":modal") | ||
| .querySelector(CSS.supports("selector(:modal)") ? ":modal" : "dialog[open]") |
There was a problem hiding this comment.
The fallback selector dialog[open] is not semantically equivalent to :modal. The :modal selector matches only dialogs opened with showModal() (which make outside content inert), while dialog[open] matches any dialog with the open attribute, including non-modal dialogs opened with show() (which don't make outside content inert). This could cause incorrect behavior where non-modal dialogs are incorrectly treated as making content outside them inert. Consider using dialog[open]:modal as a fallback (which requires the browser to parse it but still functions), or check for the modal state differently (e.g., checking for the presence of a ::backdrop pseudo-element or using JavaScript to track modal state).
See below for a potential fix:
// Element must be connected to the document.
if (!this.element.isConnected) {
return false;
}
// Elements within inert containers should not be announced.
if (this.element.closest("[inert]")) {
return false;
}
// If there is a modal element on the page, everything outside of it is implicitly inert.
// Only apply this logic when the browser supports the :modal selector.
const doc = this.element.ownerDocument;
let modal = null;
if (typeof CSS !== "undefined" && typeof CSS.supports === "function" && CSS.supports("selector(:modal)")) {
modal = doc.querySelector(":modal");
}
// If there is a modal, only announce elements within it; otherwise, allow announcement.
return modal ? modal.contains(this.element) : true;
| // This can be checked by seeing if the element is within the modal, if the modal is present. | ||
| (this.element.ownerDocument | ||
| .querySelector(":modal") | ||
| .querySelector(CSS.supports("selector(:modal)") ? ":modal" : "dialog[open]") |
There was a problem hiding this comment.
The new feature detection logic and fallback behavior for environments without :modal support lacks test coverage. Consider adding tests that verify correct behavior in environments where CSS.supports('selector(:modal)') returns false, ensuring the fallback selector works appropriately.
JSDOM doesn’t implement
:modal, so if Jest is used to test components in an environment where@github/arianotify-polyfillis loaded, it throwsSyntaxError: unknown pseudo-class selector ':modal'.This PR fixes that issue by only using
:modalwhere it’s supported.Semi-related: #25