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

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

Merged
merged 4 commits into from
Apr 4, 2023

Conversation

keithamus
Copy link
Collaborator

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

patchSelectorFn(Element.prototype, 'matches', rewriteSelector);
patchSelectorFn(Element.prototype, 'closest', rewriteSelector);
patchSelectorFn(
DocumentFragment.prototype,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ShadowRoot inherits DocumentFragment, and so patching DocumentFragment also patches ShadowRoot.

Comment on lines +57 to +52
patchSelectorFn(Element.prototype, 'matches', rewriteSelector);
patchSelectorFn(Element.prototype, 'closest', rewriteSelector);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

HTMLDocument nor DocumentFragment have matches nor closest. I think I have all functions that take CSS selectors here. Let me know if I missed one!

@keithamus
Copy link
Collaborator Author

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

src/popover.ts Outdated Show resolved Hide resolved
src/popover.ts Outdated Show resolved Hide resolved
Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

Looks fine to me, though happy for @yinonov to review if they have a chance.

src/popover.ts Outdated Show resolved Hide resolved
@keithamus keithamus marked this pull request as draft March 24, 2023 09:12
@keithamus
Copy link
Collaborator Author

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

@keithamus keithamus force-pushed the patch-qsa-matches-closest-to-accept-open branch from b7536c8 to 2e476df Compare March 28, 2023 10:05
@keithamus keithamus changed the title Patch qsa matches closest to accept :open/:closed Patch qsa matches closest to accept :popover Mar 28, 2023
@netlify
Copy link

netlify bot commented Mar 28, 2023

Deploy Preview for popover-polyfill ready!

Name Link
🔨 Latest commit 88a99eb
🔍 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 keithamus force-pushed the patch-qsa-matches-closest-to-accept-open branch from 2e476df to 5a54158 Compare March 28, 2023 10:09
@keithamus
Copy link
Collaborator Author

keithamus commented Mar 28, 2023

Looks like w3c/csswg-drafts#8637 is moving forward with :popover, so I've updated this PR. We should probably wait for whatwg/html#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 keithamus force-pushed the patch-qsa-matches-closest-to-accept-open branch 2 times, most recently from cf28b60 to 6289342 Compare April 3, 2023 09:37
@keithamus keithamus changed the title Patch qsa matches closest to accept :popover Patch qsa matches closest to accept :popover-open Apr 3, 2023
@keithamus keithamus marked this pull request as ready for review April 3, 2023 16:10
@keithamus keithamus force-pushed the patch-qsa-matches-closest-to-accept-open branch from 6289342 to 3a2446e Compare April 3, 2023 16:13
@keithamus
Copy link
Collaborator Author

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

src/popover.css Show resolved Hide resolved
@jgerigmeyer

This comment was marked as resolved.

* main:
  chore(deps): Automated dependency upgrades
  changelog
  return null if targetElement is present but disconnected
  refactor if statement
  Update popover.ts
@jgerigmeyer jgerigmeyer merged commit 5c2f18e into main Apr 4, 2023
@jgerigmeyer jgerigmeyer deleted the patch-qsa-matches-closest-to-accept-open branch April 4, 2023 14:47
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