CSS: Support custom properties, #3144 #3199

ConnorAtherton · 2016-06-25T17:32:21Z

Summary

Allows users to get and set the value of CSS custom properties on a DOM element through the .css method.

It works exactly like the example in the issue, which I am copying here for reference.

<div style="--color: red; color: var(--color)">text</div>
<script>
$('div').css('--color') ; // Returns 'red'
$('div').css('--color' ,'blue') ; // Sets to 'blue'

Fixes #3144

Checklist

All authors have signed the CLA at https://contribute.jquery.com/CLA/
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

mgol · 2016-06-26T19:19:14Z

src/css.js

 }

+function isCustomProperty( value ) {
+	return value.match( /--(.*)/ );


A few remarks:

This will match things like a--b; you should check that the string starts with --, not just contains it.

Why did you introduce the group (.*)? You don't use it anywhere.

It's better to change that to /^--.*/.test( value )` as you only care about it matching or not and what you wrote will return the array with matches; the browser has better chances of optimising the former.

mgol · 2016-06-26T19:26:41Z

This PR adds 47 bytes (minified+gzipped). That's not a negligible amount. We should explore whether it wouldn't be better to just use the setProperty method everywhere. It might affect performance so we should do some performance testing. https://jsperf.com/ is offline; we can use benchmark.js, though.

mgol · 2016-06-26T19:28:48Z

@ConnorAtherton Would you like to try to run some performance tests? You can see my example template in one of my PRs.

ConnorAtherton · 2016-06-27T22:07:57Z

@mgol Thanks for looking at this. All your comments are valid and I will rework my additions based on your feedback. Thanks for providing the link to your benchmarks in your other PR, I will update this thread with similar metrics in the coming days. I like the idea of using setProperty everywhere but will hold off on that decision until we have some metrics to consider.

ConnorAtherton · 2016-07-04T22:44:09Z

Following on from my previous comment, I have benchmarked both methods for adding styles to a DOM node. Here is my benchmark (I think this would be a useful template to add to the repo for future PRs) followed by the results:

<!DOCTYPE html>
<html>
<head>
    <meta charset="utf-8">
    <title>A page for perf tests</title>
    <style>
        /**
         * Any styles for perf start state
         */
    </style>
</head>
<body>
    <div id="test-div"></div>
    <script src="http://code.jquery.com/jquery-git.js"></script>
    <script>
        var $j = jQuery.noConflict();
    </script>
    <script src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.13.1/lodash.js"></script>
    <script src="https://cdnjs.cloudflare.com/ajax/libs/benchmark/2.1.0/benchmark.js"></script>
    <script src="../dist/jquery.js"></script>
    <script>
        var suite = new Benchmark.Suite;

        var currentVersion = $j('#test-div');
        var newVersion = $('#test-div');

        // add tests
        suite
            .add('Adding styles using object assignment', function() {
                currentVersion.css('--color', 'blue');
                currentVersion.css('--color');
            })
            .add('Adding styles using setProperty', function() {
                newVersion.css('--color', 'blue');
                newVersion.css('--color');
            })
            // add listeners
            .on('cycle', function(event) {
                console.log(String(event.target));
            })
            .on('complete', function() {
                // So we can inspect the results ourselves in the console
                window.s = this;
                console.log('Fastest is ' + this.filter('fastest').map('name'));
            })
            // run async
            .run({'async': true});
    </script>
</body>
</html>

Chrome 51:

Adding styles using object assignment x 52,588 ops/sec ±3.13% (52 runs sampled)
Adding styles using setProperty x 38,907 ops/sec ±2.29% (54 runs sampled)
Fastest is Adding styles using object assignment

Firefox 49:

Adding styles using object assignment x 94,883 ops/sec ±5.27% (16 runs sampled)
Adding styles using setProperty x 15,532 ops/sec ±10.38% (15 runs sampled)
Fastest is Adding styles using object assignment

Safari 9:

Adding styles using object assignment x 84,248 ops/sec ±4.56% (41 runs sampled) 
Adding styles using setProperty x 69,650 ops/sec ±11.84% (42 runs sampled) 
Fastest is Adding styles using object assignment

IE WIP

dmethvin · 2016-07-05T16:42:33Z

It's definitely slower, especially in Firefox for some reason, but I don't think it is too slow. Focusing on Firefox, 15,000 ops per second is one op in 66 microseconds. How many CSS properties would typical code set in a 16ms frame? Also, setting properties often causes reflow which would be a lot more expensive than the act of setting the property.

markelog · 2016-07-08T23:27:09Z

Also, setting properties often causes reflow which would be a lot more expensive than the act of setting the property.

Totally, should be fine

markelog · 2016-07-08T23:30:36Z

src/css.js

 }

+function isCustomProperty( value ) {
+	return /^--.*/.test( value );


Agreed, but the regex might as well be /^--/.

mgol · 2016-07-08T23:37:22Z

Thanks for doing the benchmarks, @ConnorAtherton! The numbers look good so I'd go with just always using setProperty/getProperty.

As an added bonus, we might get https://bugs.jquery.com/ticket/14394 fixed as a side-effect. :-) You could use a test from my PR (that had to be reverted later):
https://github.com/jquery/jquery/pull/1385/files, I think it should be passing in IE 9+ although I'm not 100% sure. That's not a blocker here, though, you might as well not do this part now, we could later check if it works and add the test if it passes.

markelog · 2016-07-08T23:38:57Z

src/css.js

-				style[ name ] = value;
-			}
+                if ( customProperty ) {
+                    style.setProperty( name, value );


Just wondering, why we can't always use setProperty?

Oh, i see, i missed discussion :)

I don't think we can use setProperty here because properties like maxWidth and marginRight don't work. I've delayed working on this because I keep running into this issue but according to the spec, and please correct me if I'm reading this wrong, camel-cased properties aren't transformed into their CSS equivalents, only converted to ASCII lowercase. Every test that fails when I switch to setProperty for every property is precisely because of this.

Sources:

https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-setproperty

https://dom.spec.whatwg.org/#converted-to-ascii-lowercase

markelog · 2016-07-08T23:41:06Z

As an added bonus, we might get https://bugs.jquery.com/ticket/14394 fixed as a side-effect. :-) You could use a test from my PR (that had to be reverted later):

That would be cool! But can we do this in separate PR?

ConnorAtherton · 2016-07-13T18:27:21Z

Hey all, thanks for the feedback. Apologies for the delay with this, I'm currently on vacation. Going to work on this more today.

ConnorAtherton · 2016-07-13T18:56:20Z

Seeing this behaviour on chrome when trying to fetch properties from the style object

mgol · 2016-07-19T18:40:00Z

@ConnorAtherton That's expected. Regular CSS properties can be read or set using regular property access but CSS variables can't, you have to use setProperty/getPropertyValue.

Do you know what needs to be done to finish this PR?

gibson042 · 2016-07-22T03:35:37Z

test/unit/css.js

+	assert.equal( jQuery( "#customPropertyTest" ).css( "--color" ), "red", "Modified CSS custom property using object: Assert value is right" );
+
+	jQuery( "#customPropertyTest" ).css( { "--theme-dark": "purple" } );
+	assert.equal( jQuery( "#customPropertyTest" ).css( "--theme-dark" ), "purple", "Modified CSS custom property with dashed name: Assert value is right" );


A mixed-case custom property test would also be welcome.

ConnorAtherton · 2016-07-22T07:49:13Z

@mgol Status update: I haven't been able to remove setProperty (see inline comment) but I have applied the test feedback and removed some stuff. All tests are passing locally for me, although I would like to check more browser coverage.

Thanks for the feedback everyone.

markelog · 2016-07-25T01:30:53Z

What about #3199 (comment) and #3199 (comment)?

mgol · 2016-10-31T18:39:37Z

@ConnorAtherton Did you look at @markelog's comments?

mgol · 2016-12-05T20:21:36Z

@ConnorAtherton A friendly ping. :)

mgol · 2016-12-14T12:34:36Z

@ConnorAtherton We'd like to get it into 3.2.0. If you don't have time to finish please let us know so we'll tweak it & land it ourselves.

Fixes jquerygh-3144 Closes jquerygh-3199

mgol · 2017-03-01T23:34:09Z

I finished the PR as #3557.

mgol · 2017-03-01T23:59:52Z

@ConnorAtherton I've finished it up for you in #3557. You'll still be credited as the commit author. Thanks for all the work that went into this PR!

Fixes jquerygh-3144 Closes jquerygh-3199 Closes jquerygh-3557

CSS: Support custom properties, jquery#3144

d90fc90

jquerybot added the CLA: Valid label Jun 25, 2016

mgol reviewed Jun 26, 2016
View reviewed changes

Add iFrame assertions for CSS custom properties

641c1a6

markelog reviewed Jul 8, 2016
View reviewed changes

timmywil added this to the 3.2.0 milestone Jul 13, 2016

gibson042 reviewed Jul 22, 2016
View reviewed changes

Test cleanup and src feedback

86005bc

JSFOwner removed the CLA: Valid label Nov 5, 2016

mgol self-assigned this Feb 13, 2017

mgol pushed a commit to mgol/jquery that referenced this pull request Mar 1, 2017

CSS: Support custom properties

8ad464d

Fixes jquerygh-3144 Closes jquerygh-3199

mgol mentioned this pull request Mar 1, 2017

CSS: Support custom properties #3557

Merged

4 tasks

mgol pushed a commit to mgol/jquery that referenced this pull request Mar 6, 2017

CSS: Support custom properties

4787025

Fixes jquerygh-3144 Closes jquerygh-3199 Closes jquerygh-3557

mgol pushed a commit to mgol/jquery that referenced this pull request Mar 7, 2017

CSS: Support custom properties

f7cb569

Fixes jquerygh-3144 Closes jquerygh-3199 Closes jquerygh-3557

mgol pushed a commit to mgol/jquery that referenced this pull request Mar 7, 2017

CSS: Support custom properties

619bf98

Fixes jquerygh-3144 Closes jquerygh-3199 Closes jquerygh-3557

mgol closed this in #3557 Mar 7, 2017

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

CSS: Support custom properties, #3144 #3199

CSS: Support custom properties, #3144 #3199

Uh oh!

Conversation

ConnorAtherton commented Jun 25, 2016 • edited by mgol Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

mgol Jun 26, 2016

Choose a reason for hiding this comment

Uh oh!

mgol commented Jun 26, 2016

Uh oh!

mgol commented Jun 26, 2016

Uh oh!

ConnorAtherton commented Jun 27, 2016

Uh oh!

ConnorAtherton commented Jul 4, 2016

Uh oh!

dmethvin commented Jul 5, 2016

Uh oh!

markelog commented Jul 8, 2016

Uh oh!

markelog Jul 8, 2016

Choose a reason for hiding this comment

Uh oh!

timmywil Jul 13, 2016

Choose a reason for hiding this comment

Uh oh!

mgol commented Jul 8, 2016

Uh oh!

markelog Jul 8, 2016

Choose a reason for hiding this comment

Uh oh!

markelog Jul 8, 2016

Choose a reason for hiding this comment

Uh oh!

ConnorAtherton Jul 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markelog commented Jul 8, 2016

Uh oh!

ConnorAtherton commented Jul 13, 2016

Uh oh!

ConnorAtherton commented Jul 13, 2016

Uh oh!

mgol commented Jul 19, 2016

Uh oh!

gibson042 Jul 22, 2016

Choose a reason for hiding this comment

Uh oh!

ConnorAtherton commented Jul 22, 2016

Uh oh!

markelog commented Jul 25, 2016

Uh oh!

mgol commented Oct 31, 2016

Uh oh!

mgol commented Dec 5, 2016

Uh oh!

mgol commented Dec 14, 2016

Uh oh!

mgol commented Mar 1, 2017

Uh oh!

mgol commented Mar 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

9 participants

ConnorAtherton commented Jun 25, 2016 •

edited by mgol

Loading

ConnorAtherton Jul 22, 2016 •

edited

Loading