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

feat(text selector): pierce shadow roots #1619

Merged
merged 1 commit into from
Apr 3, 2020
Merged

feat(text selector): pierce shadow roots #1619

merged 1 commit into from
Apr 3, 2020

Conversation

dgozman
Copy link
Contributor

@dgozman dgozman commented Apr 1, 2020

References #1375.

- Text body can also be a JavaScript-like regex wrapped in `/` symbols. This means `text=/^\\s*Login$/i` will match `<button> loGIN</button>` with any number of spaces before "Login" and no spaces after.

> **NOTE** Text engine searches for elements inside open shadow roots, but not inside closed shadow roots or iframes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we consider it a breaking change if this starts piercing closed shadow dom?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most likely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor detail: Isn't this text better left as a generic paragraph/section/note about shadow DOM, and then saying "This is only currently supported by the text engine". Reason is that you can then easily amend with "text engine, css engine" etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. I don't think css will ever support shadow dom - that would be against the css spec. We'll probably make attribute selectors pierce shadow dom as well, and I'll make this note more generic.


function queryInternal(root: SelectorRoot, matcher: Matcher): Element | undefined {
const document = root instanceof Document ? root : root.ownerDocument;
if (!document)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!document)
assert(document);

This is an assert right? ownerDocument should be never null unless root is a document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right! We don't have asserts in the injected code though, so I am not sure how to change this.

Copy link
Contributor

@JoelEinbinder JoelEinbinder Apr 2, 2020

Choose a reason for hiding this comment

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

just ownerDocument! then? The if there makes me try to reason about the null case and I go down a bad path.

@dgozman
Copy link
Contributor Author

dgozman commented Apr 2, 2020

I did not address any comments, please take another look :)

Copy link
Contributor

@JoelEinbinder JoelEinbinder left a comment

Choose a reason for hiding this comment

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

I think any time you get the opportunity to correctly use a generator you should, because then you can win arguments against people who say it is useless 😄 .

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.

3 participants