Skip to content

Conversation

@mgol
Copy link
Member

@mgol mgol commented Dec 22, 2022

Summary

Backport jQuery selection context logic to selector-native.

This makes:

$div.find("div > *")

no longer matching children of $div.

Also, leading combinators now work, e.g.:

$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 gh-5185
Ref gh-5085

The full build is at +19 bytes now. I'm sure this can be reduced a bit but some may be unavoidable unless we want to duplicate a lot of code between selector & selector-native which I'd like to avoid.

The selector-native build is at -3274 bytes compared to the full main (c66d470) build and at +1056 bytes compared to the selector-native build on that same main build.

It's quite likely more size optimizations are possible in the selector-native build, I'd appreciate some ideas. One thing I know about is the tokenize & toSelector functions could be simplified slightly as it's enough to collect string parts without all the other context information - instead of:

tokens.push( {
	value: matched,
	type: type,
	matches: match
} );

it'd be enough to:

tokens.push( matched )

and toSelector would be simplified from:

function toSelector( tokens ) {
	var i = 0,
		len = tokens.length,
		selector = "";
	for ( ; i < len; i++ ) {
		selector += tokens[ i ].value;
	}
	return selector;
}

to:

function toSelector( tokens ) {
	return tokens.join( "" );
}

That would save 37 bytes in the selector-native build but would require duplicating those two functions and especially the tokenize one is quite large so I opted against that for now.

Checklist

@mgol mgol added Selector Needs review Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Dec 22, 2022
@mgol mgol added this to the 4.0.0 milestone Dec 22, 2022
@mgol mgol requested review from gibson042 and timmywil December 22, 2022 15:09
@mgol mgol self-assigned this Dec 22, 2022
@mgol mgol removed the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Jan 12, 2023
Copy link
Member

@gibson042 gibson042 left a 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!

// as such selectors are not recognized by querySelectorAll.
// Thanks to Andrew Dupont for this technique.
if ( nodeType === 1 &&
( rdescend.test( selector ) || rcombinators.test( selector ) ) ) {
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense to me.

Copy link
Member Author

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?

Copy link
Member Author

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.

Copy link
Member Author

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:

if ( ( nid = context.getAttribute( "id" ) ) ) {
nid = jQuery.escapeSelector( nid );
} else {
context.setAttribute( "id", ( nid = jQuery.expando ) );
Copy link
Member

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?

Copy link
Member Author

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:

jquery/src/selector.js

Lines 288 to 290 in b02a257

if ( nid === expando ) {
context.removeAttribute( "id" );
}

Copy link
Member Author

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.

mgol added 3 commits February 12, 2023 00:48
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
@mgol mgol force-pushed the selector-native-context branch from e480d28 to 1df31c5 Compare February 11, 2023 23:57
@mgol mgol added the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Feb 13, 2023
@timmywil timmywil removed the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Feb 13, 2023
@mgol mgol removed the Needs review label Feb 13, 2023
@mgol mgol merged commit 2e644e8 into jquery:main Feb 13, 2023
@mgol mgol deleted the selector-native-context branch February 13, 2023 17:34
mgol added a commit to jquery/sizzle that referenced this pull request Feb 13, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Development

Successfully merging this pull request may close these issues.

Selector: Fix scoping logic of selector-native

3 participants