-
Notifications
You must be signed in to change notification settings - Fork 20.5k
Selector: Backport jQuery selection context logic to selector-native #5186
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
Conversation
gibson042
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty happy with this, modulo some nits. Thanks @mgol!
src/selector-native.js
Outdated
| // as such selectors are not recognized by querySelectorAll. | ||
| // Thanks to Andrew Dupont for this technique. | ||
| if ( nodeType === 1 && | ||
| ( rdescend.test( selector ) || rcombinators.test( selector ) ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rcombinators is a much less clear name than e.g. rleadingCombinator, but I'm weighing the costs of gratuitous divergence from Sizzle. I think it would be best to rename in both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...with Sizzle going away soon, though, perhaps it'd be fine to just rename it on main & 3.x-stable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed here; I can submit a separate PR just renaming this on 3.x-stable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, well, this is a simple change so I submitted a PR to Sizzle as well:
- jQuery
3.x-stable: Selector: Rename rcombinators to rleadingCombinator #5208 - Sizzle
main: Selector: Rename rcombinators to rleadingCombinator sizzle#494
| if ( ( nid = context.getAttribute( "id" ) ) ) { | ||
| nid = jQuery.escapeSelector( nid ); | ||
| } else { | ||
| context.setAttribute( "id", ( nid = jQuery.expando ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this id attribute removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. I removed the try-catch since we don't do custom traversal here but I should have instead changed it into try-finally to keep this finally block:
Lines 288 to 290 in b02a257
| if ( nid === expando ) { | |
| context.removeAttribute( "id" ); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gibson042 The finally block added.
This makes:
```js
$div.find("div > *")
```
no longer matching children of `$div`.
Also, leading combinators now work, e.g.:
```js
$div.find( "> *" );
```
returns children of `$div`.
As a result of that, a number of tests are no longer skipped in the
`selector-native` mode.
Fixes jquerygh-5185
Ref jquerygh-5085
e480d28 to
1df31c5
Compare
Summary
Backport jQuery selection context logic to
selector-native.This makes:
no longer matching children of
$div.Also, leading combinators now work, e.g.:
returns children of
$div.As a result of that, a number of tests are no longer skipped in the
selector-nativemode.Fixes gh-5185
Ref gh-5085
The full build is at
+19 bytesnow. I'm sure this can be reduced a bit but some may be unavoidable unless we want to duplicate a lot of code betweenselector&selector-nativewhich I'd like to avoid.The
selector-nativebuild is at-3274 bytescompared to the fullmain(c66d470) build and at+1056 bytescompared to theselector-nativebuild on that samemainbuild.It's quite likely more size optimizations are possible in the
selector-nativebuild, I'd appreciate some ideas. One thing I know about is thetokenize&toSelectorfunctions could be simplified slightly as it's enough to collect string parts without all the other context information - instead of:it'd be enough to:
and
toSelectorwould be simplified from:to:
That would save
37 bytesin theselector-nativebuild but would require duplicating those two functions and especially thetokenizeone is quite large so I opted against that for now.Checklist