Skip to content

Selector: Allow whitespace after hex escapes in CSS identifiers #450

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.

Sign up for GitHub

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

Jump to bottom
Closed
gibson042 wants to merge 4 commits into from
Closed

Selector: Allow whitespace after hex escapes in CSS identifiers #450

gibson042 wants to merge 4 commits into from

Conversation

@gibson042
Copy link
Member

@gibson042 gibson042 commented Jul 1, 2019

Ref jquery/jquery#4424

   raw     gz Sizes
 66770  19908 dist/sizzle.js
 19893   7398 dist/sizzle.min.js

   raw     gz Compared to master @ 9ffbb8b6e18d79bdc8cc9ee94d7b923de0661b32
   +92    +35 dist/sizzle.js
   +48     -3 dist/sizzle.min.js

@gibson042 gibson042 requested review from mgol and timmywil July 1, 2019 19:38
timmywil
timmywil approved these changes Jul 1, 2019
View reviewed changes
Copy link
Member

@timmywil timmywil left a comment

Choose a reason for hiding this comment

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

I can't say I fully understand the unescaping, but what I can gleam makes sense. If the tests pass, I'm down 👍

@mgol
Copy link
Member

mgol commented Jul 1, 2019

Remember to port the new tests to jQuery as well.

mgol
mgol reviewed Jul 15, 2019
View reviewed changes
Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

Regex changes in source look good to me according to the linked token diagram. I have a few questions.

src/sizzle.js
var high = "0x" + escaped - 0x10000;

// NaN means non-codepoint
// Support: Firefox<24
Copy link
Member

@mgol mgol Jul 15, 2019

Choose a reason for hiding this comment

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

Is that old Firefox workaround no longer needed considering that Sizzle doesn't drop old browsers?

Copy link
Member Author

@gibson042 gibson042 Jul 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

No, it's just that the new code doesn't encounter that bug because high is not used unless nonHex is falsy (i.e., because we're replacing a nonempty hexadecimal escape).

Copy link
Member

@mgol mgol Jul 29, 2019

Choose a reason for hiding this comment

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

@gibson042 Interesting. Was the NaN check for high even needed before since escapedWhitespace was checked as a fallback anyway (as far as I understand the previous logic)?

mgol
mgol approved these changes Jul 29, 2019
View reviewed changes
Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

One question but LGTM anyway.

mgol
mgol reviewed Jul 29, 2019
View reviewed changes
test/unit/selector.js Outdated
broken( "Broken Selector", "," );
broken( "Broken Selector", ",a" );
broken( "Broken Selector", "a," );
broken( "Identifer with bad escape", "foo\\\fbaz" );
Copy link
Member

@mgol mgol Jul 29, 2019

Choose a reason for hiding this comment

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

There's a typo here:

Suggested change
broken( "Identifer with bad escape", "foo\\\fbaz" );
broken( "Identifier with bad escape", "foo\\\fbaz" );

@mgol
Copy link
Member

mgol commented Jul 29, 2019

I incorporated this PR into jquery/jquery#4395. Of course, this would still be good to land in Sizzle so that we can release the fix in jQuery 3.4.2.

@mgol mgol mentioned this pull request Aug 12, 2019
Closed
@gibson042 gibson042 closed this in b2b650b Aug 19, 2019
@mgol mgol mentioned this pull request Aug 19, 2019
Closed
@mgol
Copy link
Member

mgol commented Aug 19, 2019 •
edited
Loading

One of the added tests fails in Firefox 3.6: #454

gibson042 added a commit to gibson042/jquery that referenced this pull request Aug 20, 2019
@gibson042
Tests: Port changes from Sizzle
521066d 
Ref jquery/sizzle#450
gibson042 added a commit to gibson042/jquery that referenced this pull request Aug 20, 2019
@gibson042
Tests: Port changes from Sizzle
25b4b97 
Ref jquery/sizzle#450
@gibson042 gibson042 mentioned this pull request Aug 20, 2019
Merged
4 tasks
gibson042 added a commit to jquery/jquery that referenced this pull request Aug 20, 2019
@gibson042
Tests: Port changes from Sizzle
ac5f7cd 
Ref jquery/sizzle#450
Closes gh-4464
@mgol mgol mentioned this pull request Aug 21, 2019
Closed
@mgol mgol added this to the 2.3.5 milestone Feb 28, 2020
mgol added a commit to mgol/jquery that referenced this pull request Sep 8, 2022
@mgol
Selector: Allow whitespace after hex escapes in CSS identifiers
85b60f4 
Ref jquery#4424
Ref jquery/sizzle#450
mgol added a commit to mgol/jquery that referenced this pull request Sep 12, 2022
@mgol
Selector: Allow whitespace after hex escapes in CSS identifiers
9c1550e 
Ref jquery#4424
Ref jquery/sizzle#450
mgol added a commit to mgol/jquery that referenced this pull request Sep 19, 2022
@mgol
Selector: Allow whitespace after hex escapes in CSS identifiers
7cd75da 
Ref jquery#4424
Ref jquery/sizzle#450
mgol added a commit to mgol/jquery that referenced this pull request Sep 21, 2022
@mgol
Selector: Allow whitespace after hex escapes in CSS identifiers
c492967 
Ref jquery#4424
Ref jquery/sizzle#450
mgol added a commit to mgol/jquery that referenced this pull request Oct 3, 2022
@mgol
Selector: Allow whitespace after hex escapes in CSS identifiers
abc8c86 
Ref jquery#4424
Ref jquery/sizzle#450
mgol added a commit to mgol/jquery that referenced this pull request Oct 4, 2022
@mgol
Selector: Allow whitespace after hex escapes in CSS identifiers
3b11d8e 
Ref jquery#4424
Ref jquery/sizzle#450
mgol added a commit to mgol/jquery that referenced this pull request Nov 17, 2022
@mgol
Selector: Allow whitespace after hex escapes in CSS identifiers
9a04815 
Ref jquery#4424
Ref jquery/sizzle#450
mgol added a commit to mgol/jquery that referenced this pull request Dec 1, 2022
@mgol
Selector: Allow whitespace after hex escapes in CSS identifiers
5ed1ab9 
Ref jquery#4424
Ref jquery/sizzle#450
mgol added a commit to mgol/jquery that referenced this pull request Dec 13, 2022
@mgol
Selector: Allow whitespace after hex escapes in CSS identifiers
ebb312e 
Ref jquery#4424
Ref jquery/sizzle#450
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgol mgol mgol approved these changes

@timmywil timmywil timmywil approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

2.3.5

Development

Successfully merging this pull request may close these issues.

3 participants

@gibson042 @mgol @timmywil