oddbird / popover-polyfill

Polyfills the HTML popover attribute and showPopover/hidePopover/togglePopover methods onto HTMLElement, as well as the popovertarget and popovertargetaction attributes on <button> elements.
https://popover.oddbird.net
BSD 3-Clause "New" or "Revised" License
269 stars 14 forks source link

Patch qsa matches closest to accept `:popover-open` #84

Closed keithamus closed 1 year ago

keithamus commented 1 year ago

A potentially controversial change.

By default running something like el.matches(':open') on a browser without native Popover will result in an exception:

Chrome:

DOMException: Failed to execute 'matches' on 'Element': ':open' is not a valid selector.

Firefox:

DOMException: Element.matches: ':open' is not a valid selector

This means users of this polyfill need to work around this; either by using a try/catch, or by doing feature detection up front, e.g.:

let openSelector = ':open'
try {
  document.querySelector(openSelector)
} catch {
  // use polyfilled selector instead
  openSelector = '.\\:open'
}

This is a little cumbersome and has to be done for every selector you intend to use, and given we're patching prototype methods anyway, perhaps it could be useful for us to patch the CSS selector functions (I believe I have them all, querySelector, querySelectorAll, matches, closest).

This change patches all of those functions taking the selector string given and replacing instances of :open with .\\:open and :closed with :not(.\\:open). This allows us to use the :open/:closed selector and have it work with the polyfill.

Steps to test/reproduce

Please explain how to best reproduce the issue and/or test the changes locally (including the pages/URLs/views/states to review).

Tests have been added to ensure qsa/matches/closed work but it doesn't do anything close to exhaustive input testing. I've manually tested the replacer function with various inputs. We could add some unit tests to ensure its robustness, but here's how I validated it:

const nonEscapedOpenSelector = /(^|[^\\]):open\b/g;
const nonEscapedClosedSelector = /(^|[^\\]):closed\b/g;
  function rewriteSelector(selector) {
    return selector
      .replace(nonEscapedOpenSelector, '$1.\\:open')
      .replace(nonEscapedClosedSelector, '$1:not(.\\:open)');
  }
console.assert(rewriteSelector(':open') == '.\\:open')
console.assert(rewriteSelector('.\\:open') == '.\\:open')
console.assert(rewriteSelector('[bar]:open') == '[bar].\\:open')
console.assert(rewriteSelector('[bar]:opened') == '[bar]:opened')
console.assert(rewriteSelector('[bar]:open.\\:open:open') == '[bar].\\:open.\\:open.\\:open')
console.assert(rewriteSelector(':closed') == ':not(.\\:open)')
console.assert(rewriteSelector('.\\:closed') == '.\\:closed')

Show me

Provide screenshots/animated gifs/videos if necessary.

REMEMBER: Attach this PR to the Trello card

keithamus commented 1 year ago

/cc @yinonov who did a lot of work on shadowroots here. What do you think of this change?

keithamus commented 1 year ago

Pausing this one for now as https://github.com/w3c/csswg-drafts/issues/8637 may mean we need to change the pseudo selector to :popover.

netlify[bot] commented 1 year ago

Deploy Preview for popover-polyfill ready!

Name Link
Latest commit 88a99ebbcf26d200b75f64f7b569726dd4bcd63e
Latest deploy log https://app.netlify.com/sites/popover-polyfill/deploys/642c37e774f9370008d2f285
Deploy Preview https://deploy-preview-84--popover-polyfill.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

keithamus commented 1 year ago

Looks like https://github.com/w3c/csswg-drafts/issues/8637 is moving forward with :popover, so I've updated this PR. We should probably wait for https://github.com/whatwg/html/pull/9077 to land before we merge this.

Chrome implementation for this is here: https://chromium-review.googlesource.com/c/chromium/src/+/4373888. We should also wait for this to land IMO.

keithamus commented 1 year ago

Upstream PRs have landed. :popover-open is the new selector. We should consider re-reviewing this PR now.

jgerigmeyer commented 1 year ago

Add note to changelog?