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.

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

The compatibility of different browsers should be considered about test case css(--customProperty) #4926

Closed
fecore1 opened this issue Sep 7, 2021 · 13 comments · Fixed by #4930
Closed

The compatibility of different browsers should be considered about test case css(--customProperty) #4926

fecore1 opened this issue Sep 7, 2021 · 13 comments · Fixed by #4930
Labels
CSS Has Pull Request
Milestone
3.6.1

Comments

@fecore1
Copy link
Contributor

fecore1 commented Sep 7, 2021

Description

The compatibility of different browsers should be considered about test case css(--customProperty)

$elem.css( "--prop2" ) has different val on different browsers:

  • firefox 91: $elem.css( "--prop2" ) => 'val2'
  • chrome 93: $elem.css( "--prop2" ) => ' val2'
  • edge 93: $elem.css( "--prop2" ) => ' val2'

As i know, jQuery.css is based on getComputedStyle and getPropertyValue, and that have different behaves on different browsers.

Link to test case

Please open test case bellow use different browsers and attention the value of --prop2

https://codepen.io/fecore1/pen/KKqNxQx
@mgol
Copy link
Member

mgol commented Sep 7, 2021 •
edited
Loading

Thanks for the report. There was a recent spec change that require CSS Custom Properties values to be trimmed and it's just been implemented in Firefox 91 so far. I expect it to take some time to roll out to all supported browsers, especially Safari so maybe we should, indeed, trim it until all supported browsers adjust. It sounds easy enough to do, just trim the value.

BTW, Firefox behavior is still buggy as it only trims leading spaces and leaves trailing spaces as they are, they have a bug for this: https://bugzilla.mozilla.org/show_bug.cgi?id=1728165.

Would you like to try to submit a fix by yourself?

@mgol mgol added CSS Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Sep 7, 2021
@mgol
Copy link
Member

mgol commented Sep 7, 2021

BTW, we have tests for whitespace handling of CSS Custom Properties (a.k.a. CSS Variables) and the Firefox 91 change is why our Travis tests are failing: https://app.travis-ci.com/github/jquery/jquery/jobs/535647620

@timmywil
Copy link
Member

timmywil commented Sep 7, 2021

I'm in favor of trimming if that's what all browsers will do eventually.

@fecore1
Copy link
Contributor Author

fecore1 commented Sep 8, 2021

@mgol Thanks for the quick response.

There was a recent spec change that require CSS Custom Properties values to be trimmed and it's just been implemented in Firefox 91 so far.

Yes, so in my option, test case css(--customProperty) is more appropriate to be modified to assert.equal( $elem.css( "--prop2" ), "val2", "Trim whitespace" ); because Travis tests has failed now and all browsers will implement the spec change eventually.

@mgol
Copy link
Member

mgol commented Sep 8, 2021

@fecore1 Yes, that sounds correct. Would you like to submit a PR with such a change?

@mgol mgol mentioned this issue Sep 9, 2021
Merged
4 tasks
@mgol
Copy link
Member

mgol commented Sep 10, 2021

@fecore1 Hey, since this breaks our CI, we'd rather have it fixed quickly. If you're not signing up to provide a PR, that's fine, but it'd be nice if you made it clear. So far I asked twice but you haven't replied.

@fecore1
Copy link
Contributor Author

fecore1 commented Sep 13, 2021

@mgol Sorry for the late reply because I'm on a business trip these days. I will try to provide a fix PR

@mgol
Copy link
Member

mgol commented Sep 13, 2021

@fecore1 Cool! Let me know if you need any help.

fecore1 added a commit to fecore1/jquery that referenced this issue Sep 13, 2021
@fecore1
CSS: workaround trim whitespace jquery#4926
04d7e47
@fecore1 fecore1 mentioned this issue Sep 13, 2021
Merged
4 tasks
fecore1 added a commit to fecore1/jquery that referenced this issue Sep 17, 2021
@fecore1
Tests: add test case to css custom property
af5f56e 
[jquery#4926](jquery#4926)
@mgol mgol added this to the 3.6.1 milestone Sep 17, 2021
@timmywil timmywil added Has Pull Request and removed Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Sep 20, 2021
mgol pushed a commit that referenced this issue Sep 23, 2021
@fecore1
CSS: Trim whitespace surrounding CSS Custom Properties values
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
mgol pushed a commit to mgol/jquery that referenced this issue Sep 23, 2021
@fecore1 @mgol
CSS: Trim whitespace surrounding CSS Custom Properties values
d2df468 
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 added a commit to mgol/jquery that referenced this issue Sep 24, 2021
@mgol
CSS: Don't special-case backslashes when trimming CSS Custom Properties
2c120c1 
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 mgol mentioned this issue Sep 24, 2021
Closed
3 tasks
mgol pushed a commit to mgol/jquery that referenced this issue Sep 24, 2021
@fecore1 @mgol
CSS: Trim whitespace surrounding CSS Custom Properties values
ad3ea00 
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)
@mgol
Copy link
Member

mgol commented Sep 24, 2021

Link to that spec change issue: w3c/csswg-drafts#774

@mgol
Copy link
Member

mgol commented Sep 27, 2021

Reopening until we discuss whether the currently landed solution is complete or if it needs more iterations and until we cherry-pick the change to 3.x-stable. See #4938 (comment) for more context.

@mgol mgol reopened this Sep 27, 2021
@mgol mgol added the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Sep 27, 2021
mgol pushed a commit that referenced this issue Oct 18, 2021
@fecore1 @mgol
CSS: Trim whitespace surrounding CSS Custom Properties values
219ccf5 
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)
@mgol
Copy link
Member

mgol commented Oct 18, 2021

We settled on leaving the current solution as-is. We thought there are some issues with using the wrong CSS whitespace type but it looks like it works just fine.

The PR landed on main in efadfe9 and on 3.x-stable in 219ccf5 (with a followup hotfix in 509eeb8).

Thanks, @fecore1!

@mgol mgol removed the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Oct 18, 2021
@mgol mgol closed this as completed Oct 18, 2021
mgol added a commit to mgol/jquery that referenced this issue Oct 4, 2022
@mgol
CSS: Don't special-case backslashes when trimming CSS Custom Properties
e8a5567 
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 added a commit to mgol/jquery that referenced this issue Jul 10, 2023
@mgol
CSS: Don't special-case backslashes when trimming CSS Custom Properties
df07b91 
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 added a commit to mgol/jquery that referenced this issue Jul 10, 2023
@mgol
WIP CSS: Don't special-case backslashes when trimming CSS Custom Prop…
124cdf6 
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
CSS Has Pull Request
Milestone
3.6.1
Development

Successfully merging a pull request may close this issue.

CSS: Trim whitespace surrounding CSS Custom Properties values fecore1/jquery
4 participants
@timmywil @mgol @fecore1 and others