CSS: Trim whitespace surrounding CSS Custom Properties values #4930

fecore1 · 2021-09-13T17:22:47Z

Summary

There was a recent spec change that require CSS Custom Properties values to be trimmed, and workaround trim it until all supported browsers adjust.

Fixes #4926

Checklist

All authors have signed the CLA at https://cla.js.foundation/jquery/jquery
New tests have been added to show the fix or feature works
Grunt build and unit tests pass locally with these changes
If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com

timmywil · 2021-09-13T19:05:28Z

Don't we want to limit trimming to custom vars? I'm not sure it matters, but this trims all values.

mgol

Thanks for the PR! This is very close to what we need, see comments for the blockers.

We're also considering whether to trim in the setter as well. We can consider this separately to this PR, though, especially that we haven't reached a decision there yet.

test/unit/css.js

src/css/curCSS.js

linux-foundation-easycla · 2021-09-14T02:20:07Z

The committers are authorized under a signed CLA.

✅ fecore1 (04d7e47, b6a958e)

fecore1 · 2021-09-14T02:49:55Z

The committers are authorized under a signed CLA.

✅ fecore1 (04d7e47, b6a958e, af5f56e)

mgol · 2021-09-14T06:48:52Z

@fecore1 yes; we had to change the CLA system we use; see #4931 for more details.

mgol · 2021-09-16T16:26:30Z

@fecore1 Can you sign the new CLA? Unfortunately, as outlined in #4931, we're not allowed to merge any new contribution (which includes updates to existing PRs) without such a signature.

[jquery#4926](jquery#4926)

fecore1 · 2021-09-17T02:26:52Z

@fecore1 Can you sign the new CLA? Unfortunately, as outlined in #4931, we're not allowed to merge any new contribution (which includes updates to existing PRs) without such a signature.

done

mgol

Looks awesome, thanks!

It's at +14 bytes now which is quite reasonable.

gibson042 · 2021-09-20T16:12:07Z

test/unit/css.js

+		"        --prop6: val6 ;\n" +
+		"        --prop7:   val7   ;\n" +
+		"        --prop8:\"val8\";\n" +
+		"        --prop9:'val9';\n" +


Should there be test cases with CSS whitespace other than U+0020 SPACE?

Good point. That also makes me think, we should perhaps replace CSS whitespace instead of calling .trim(), shouldn't we?

Yes, definitely.

I'd be good to have a test case with some Unicode whitespace that is not CSS whitespace to confirm the new planned behavior.

mgol

One comment about trim plus more test cases needed.

mgol · 2021-09-21T22:54:18Z

src/css/curCSS.js


 	computed = computed || getStyles( elem );

 	// getPropertyValue is needed for `.css('--customProperty')` (gh-3144)
 	if ( computed ) {
 		ret = computed.getPropertyValue( name ) || computed[ name ];

+		// trim whitespace for custom property (issue #4926)
+		if ( isCustomProp ) {
+			ret = ret.trim();


@fecore1 As @gibson042 rightly noticed, we can't actually call .trim() as that removes all Unicode whitespace:
https://262.ecma-international.org/12.0/#sec-trimstring
https://262.ecma-international.org/12.0/#prod-WhiteSpace
We need to only remove the CSS whitespace for which we already have a regex in src/selector/var/whitespace.js, please use it.

This gotcha is a reason why we don't use String#trim at all in jQuery anymore but we only rely on this regex.

wow, thanks @gibson042 , a very professional opinion, and i will fix it as soon as possible

mgol · 2021-09-21T22:55:06Z

test/unit/css.js

+		"        --prop6: val6 ;\n" +
+		"        --prop7:   val7   ;\n" +
+		"        --prop8:\"val8\";\n" +
+		"        --prop9:'val9';\n" +


I'd be good to have a test case with some Unicode whitespace that is not CSS whitespace to confirm the new planned behavior.

https://www.w3.org/TR/css-syntax-3/#whitespace

mgol

It's hard to believe but this PR is actually smaller than main and by 21 bytes! 😮 Even the non-gzipped minified version is smaller.

Just one tiny comment fix I'll apply myself, the code looks good to me. Thanks for following up with our review remarks!

mgol · 2021-09-23T10:59:03Z

src/selector.js

@@ -5,8 +5,9 @@ import documentElement from "./var/documentElement.js";
 import indexOf from "./var/indexOf.js";
 import pop from "./var/pop.js";
 import push from "./var/push.js";
-import whitespace from "./selector/var/whitespace.js";
+import whitespace from "./var/whitespace.js";


Good call, it's way more than selector-related now.

mgol · 2021-09-23T10:59:24Z

src/selector.js

@@ -71,7 +72,6 @@ var i,

 	// Leading and non-escaped trailing whitespace, capturing some non-whitespace characters preceding the latter
 	rwhitespace = new RegExp( whitespace + "+", "g" ),
-	rtrim = new RegExp( "^" + whitespace + "+|((?:^|[^\\\\])(?:\\\\.)*)" + whitespace + "+$", "g" ),


Nice that you found it!

src/css/curCSS.js

The spec has recently changed and CSS Custom Properties values are trimmed now. This change makes jQuery polyfill that new behavior for all browsers. Ref w3c/csswg-drafts#774 Fixes jquerygh-4926 Closes jquerygh-4930 (partially cherry picked from efadfe9)

mgol · 2021-09-23T13:49:26Z

Ouch, I was too quick with merging that last iteration. The rtrim regex from the selector module is quite selector-specific and it's more complex than what we need for CSS custom properties values. I'll submit a followup PR with a fix.

The original logic from PR jquerygh-4930 re-used the `rtrim` regex from the selector module but selectors have special escaping rules which means whitespace following a backslash character would not get trimmed. This change splits the logic at the unfortunate but necessary cost of size. Ref jquerygh-4926 Ref jquerygh-4930

mgol · 2021-09-24T12:18:31Z

A followup PR: #4938.

The spec has recently changed and CSS Custom Properties values are trimmed now. This change makes jQuery polyfill that new behavior for all browsers. Ref w3c/csswg-drafts#774 Fixes jquerygh-4926 Closes jquerygh-4930 Closes jquerygh-4936 (partially cherry picked from efadfe9)

The spec has recently changed and CSS Custom Properties values are trimmed now. This change makes jQuery polyfill that new behavior for all browsers. Ref w3c/csswg-drafts#774 Fixes gh-4926 Closes gh-4930 (partially cherry picked from commit efadfe9)

andersk · 2022-09-03T03:06:31Z

src/css/curCSS.js


 	computed = computed || getStyles( elem );

 	// getPropertyValue is needed for `.css('--customProperty')` (gh-3144)
 	if ( computed ) {
 		ret = computed.getPropertyValue( name ) || computed[ name ];

+		// trim whitespace for custom property (issue gh-4926)
+		if ( isCustomProp ) {
+			ret = ret.replace( rtrim, "$1" );


This throws TypeError: ret is undefined when the property isn’t set: #5105.

The original logic from PR jquerygh-4930 re-used the `rtrim` regex from the selector module but selectors have special escaping rules which means whitespace following a backslash character would not get trimmed. This change splits the logic at the unfortunate but necessary cost of size. Ref jquerygh-4926 Ref jquerygh-4930

…erties The original logic from PR jquerygh-4930 re-used the `rtrim` regex from the selector module but selectors have special escaping rules which means whitespace following a backslash character would not get trimmed. This change splits the logic at the unfortunate but necessary cost of size. Ref jquerygh-4926 Ref jquerygh-4930

CSS: workaround trim whitespace jquery#4926

04d7e47

mgol requested changes Sep 13, 2021

View reviewed changes

test/unit/css.js Outdated Show resolved Hide resolved

src/css/curCSS.js Outdated Show resolved Hide resolved

CSS: add limit to trim whitespace only for custom prop

b6a958e

Tests: add test case to css custom property

af5f56e

[jquery#4926](jquery#4926)

mgol approved these changes Sep 17, 2021

View reviewed changes

mgol added the Discuss in Meeting label Sep 17, 2021

mgol added this to the 3.6.1 milestone Sep 17, 2021

mgol added CSS Needs review labels Sep 17, 2021

timmywil removed the Discuss in Meeting label Sep 20, 2021

gibson042 reviewed Sep 20, 2021

View reviewed changes

mgol requested changes Sep 21, 2021

View reviewed changes

CSS: just trim css whitespace

29384e6

https://www.w3.org/TR/css-syntax-3/#whitespace

mgol approved these changes Sep 23, 2021

View reviewed changes

Update src/css/curCSS.js

b42af36

mgol removed the Needs review label Sep 23, 2021

mgol changed the title ~~CSS: workaround trim whitespace #4926~~ CSS: Trim whitespace surrounding CSS Custom Properties values Sep 23, 2021

mgol merged commit efadfe9 into jquery:main Sep 23, 2021

mgol mentioned this pull request Sep 24, 2021

CSS: Don't special-case backslashes when trimming CSS Custom Properties #4938

Closed

3 tasks

andersk mentioned this pull request Sep 3, 2022

.css("--custom") throws TypeError: ret is undefined (3.6.0 → 3.6.1 regression) #5105

Closed

andersk reviewed Sep 3, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSS: Trim whitespace surrounding CSS Custom Properties values #4930

CSS: Trim whitespace surrounding CSS Custom Properties values #4930

fecore1 commented Sep 13, 2021

timmywil commented Sep 13, 2021

mgol left a comment

linux-foundation-easycla bot commented Sep 14, 2021 •

edited

Loading

fecore1 commented Sep 14, 2021 •

edited by linux-foundation-easycla bot

Loading

mgol commented Sep 14, 2021

mgol commented Sep 16, 2021

fecore1 commented Sep 17, 2021

mgol left a comment

gibson042 Sep 20, 2021

mgol Sep 20, 2021

gibson042 Sep 20, 2021

mgol Sep 21, 2021

mgol left a comment

mgol Sep 21, 2021

fecore1 Sep 22, 2021

mgol Sep 21, 2021

mgol left a comment

mgol Sep 23, 2021

mgol Sep 23, 2021

mgol commented Sep 23, 2021

mgol commented Sep 24, 2021

andersk Sep 3, 2022

CSS: Trim whitespace surrounding CSS Custom Properties values #4930

CSS: Trim whitespace surrounding CSS Custom Properties values #4930

Conversation

fecore1 commented Sep 13, 2021

Summary

Checklist

timmywil commented Sep 13, 2021

mgol left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Sep 14, 2021 • edited Loading

fecore1 commented Sep 14, 2021 • edited by linux-foundation-easycla bot Loading

mgol commented Sep 14, 2021

mgol commented Sep 16, 2021

fecore1 commented Sep 17, 2021

mgol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgol commented Sep 23, 2021

mgol commented Sep 24, 2021

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Sep 14, 2021 •

edited

Loading

fecore1 commented Sep 14, 2021 •

edited by linux-foundation-easycla bot

Loading