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.

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

Request: support for CSS custom properties #3144

Closed
Getfree opened this Issue Jun 6, 2016 · 7 comments

Comments

Projects
None yet
4 participants
@Getfree

Getfree commented Jun 6, 2016

Now that CSS custom properties are supported by most browsers (except for IE), please consider adding support for them in the .css() method.

Example:

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

//without jQuery we must currently do it like this:
getComputedStyle($('div')[0]).getPropertyValue('--color') ;
$('div')[0].style.setProperty('--color','blue') ;
</script>

JsFiddle here

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jun 6, 2016

Member

Adding support shouldn't be that hard. The core issue, from what I see, is that while the regular properties can be accessed & written to via the node.style[property] property this doesn't apply to CSS Custom Properties, for them we'd need node.style.getPropertyValue(property) & node.style.setProperty(property, newValue).

The bigger issues is if it won't decrease performance for other properties. If it does we could apply special logic that would use the methods only if the name starts with --.

I'd love to have this supported but we need to be careful.

Thanks for the report!

Member

mgol commented Jun 6, 2016

Adding support shouldn't be that hard. The core issue, from what I see, is that while the regular properties can be accessed & written to via the node.style[property] property this doesn't apply to CSS Custom Properties, for them we'd need node.style.getPropertyValue(property) & node.style.setProperty(property, newValue).

The bigger issues is if it won't decrease performance for other properties. If it does we could apply special logic that would use the methods only if the name starts with --.

I'd love to have this supported but we need to be careful.

Thanks for the report!

@Getfree

This comment has been minimized.

Show comment
Hide comment
@Getfree

Getfree Jun 6, 2016

node.style.getPropertyValue(property) won't work because that only gives the properties set on the element itself.
You have to do getComputedStyle(node).getPropertyValue(property).

But that's what jQuery is already doing for normal properties anyways, because .css() gives computed property values, not directly set values.

Getfree commented Jun 6, 2016

node.style.getPropertyValue(property) won't work because that only gives the properties set on the element itself.
You have to do getComputedStyle(node).getPropertyValue(property).

But that's what jQuery is already doing for normal properties anyways, because .css() gives computed property values, not directly set values.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jun 6, 2016

Member

Yes, I meant style, not node.style which was a placeholder for any CSSStyleDeclaration, sorry. For the getter we use getComputedStyle but for setting we use node.style[property], obviously.

Member

mgol commented Jun 6, 2016

Yes, I meant style, not node.style which was a placeholder for any CSSStyleDeclaration, sorry. For the getter we use getComputedStyle but for setting we use node.style[property], obviously.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jun 9, 2016

Member

Seems pretty doable to me as well. I like it!

Member

dmethvin commented Jun 9, 2016

Seems pretty doable to me as well. I like it!

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jun 9, 2016

Member

@Getfree Would you like to try to prepare a pull request?

Member

mgol commented Jun 9, 2016

@Getfree Would you like to try to prepare a pull request?

@Getfree

This comment has been minimized.

Show comment
Hide comment
@Getfree

Getfree Jun 9, 2016

Honestly, I wouldn't feel comfortable doing that.
I haven't look into the source code, but I suspect it won't be as simple as writing a wrapper around the native JS API.
My understanding is that jQuery is supposed to be an extra layer that abstracts away browser differences and incompatibilities. And I don't know how you guys handle cases like this where a feature is missing in some browsers (IE in this case).
There's also the different branches that target some browsers and not others, like the jQuery branch that supports IE 6,7,8 and the one that doesn't.

Getfree commented Jun 9, 2016

Honestly, I wouldn't feel comfortable doing that.
I haven't look into the source code, but I suspect it won't be as simple as writing a wrapper around the native JS API.
My understanding is that jQuery is supposed to be an extra layer that abstracts away browser differences and incompatibilities. And I don't know how you guys handle cases like this where a feature is missing in some browsers (IE in this case).
There's also the different branches that target some browsers and not others, like the jQuery branch that supports IE 6,7,8 and the one that doesn't.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jun 20, 2016

Member

Let's try this out, but it won't be treated as a blocker.

Member

timmywil commented Jun 20, 2016

Let's try this out, but it won't be treated as a blocker.

@timmywil timmywil removed the Needs review label Jun 20, 2016

@timmywil timmywil added this to the 3.1.0 milestone Jun 20, 2016

ConnorAtherton added a commit to ConnorAtherton/jquery that referenced this issue Jun 25, 2016

@ConnorAtherton ConnorAtherton referenced this issue Jun 29, 2016

Closed

CSS: Support custom properties, #3144 #3199

3 of 4 tasks complete

@mgol mgol self-assigned this Feb 13, 2017

mgol added a commit to mgol/jquery that referenced this issue Mar 1, 2017

@mgol mgol referenced this issue Mar 1, 2017

Merged

CSS: Support custom properties #3557

4 of 4 tasks complete

mgol added a commit to mgol/jquery that referenced this issue Mar 6, 2017

mgol added a commit to mgol/jquery that referenced this issue Mar 7, 2017

mgol added a commit to mgol/jquery that referenced this issue Mar 7, 2017

@mgol mgol closed this in #3557 Mar 7, 2017

@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.