Cannot set property "grid-column-start" using .css() #4007

enbo · 2018-03-13T07:40:13Z

Description

$(selector).css("grid-column-start", value); fails to add an inline style to the element.

I believe there are other grid properties that have the same problem.

Link to test case

The text was updated successfully, but these errors were encountered:

mgol · 2018-03-13T09:00:23Z

Thanks for the report (and a test case!). This is because, unfortunately, jQuery adds the px suffix by default to number values and the browser discards such a value as it's invalid for grid-column-start. We plan to change it but it's a breaking change so it has to wait for 4.0 (see #2795).

We keep a blacklist of CSS properties that shouldn't be getting this treatment:

jquery/src/css.js

Lines 219 to 233 in 775caeb

    
           cssNumber: { 
        
           	"animationIterationCount": true, 
        
           	"columnCount": true, 
        
           	"fillOpacity": true, 
        
           	"flexGrow": true, 
        
           	"flexShrink": true, 
        
           	"fontWeight": true, 
        
           	"lineHeight": true, 
        
           	"opacity": true, 
        
           	"order": true, 
        
           	"orphans": true, 
        
           	"widows": true, 
        
           	"zIndex": true, 
        
           	"zoom": true 
        
           },

We'll need to add Grid properties with number values there. Would you like to submit a PR to add this value (and perhaps more Grid-related ones)?

Before a fixed jQuery version arrives you can workaround this locally by:

jQuery.cssNumber.gridColumnStart = true;

See a fixed JSFiddle: https://jsfiddle.net/j15q5tzg/42/

mgol · 2018-03-13T09:15:58Z

BTW, there's also a more obvious workaround - if you pass the string "2" instead of the number 2, it works:
https://jsfiddle.net/j15q5tzg/43/

enbo · 2018-03-13T20:47:46Z

Ah, I suspected something along those lines. Thank you for the detailed explanation and multiple workarounds!

Am I supposed to close this issue or is that what someone else should be doing?

enbo · 2018-03-13T20:51:38Z

I can submit a pull request sometime this week I think. (I think that's what you mean by PR?)

mgol · 2018-03-13T21:15:28Z

Yes, I meant “pull request”, we often shorten it to PR. There’s no hurry so next week is totally fine! Let us know if you can’t find time or if you have any questions about how to submit a PR for this issue. We’ll happily guide you through the contribution process. Thanks! -- Michał Gołębiowski-Owczarek

mgol · 2018-03-13T21:16:13Z

(And let’s keep this issue open until it’s fixed) -- Michał Gołębiowski-Owczarek

enbo · 2018-03-28T22:01:15Z

I finally got around to compiling the CSS grid properties that need to be added.

enbo@078ed8d

I'm not sure if I need to update the unit test. The current unit test doesn't seem to have any specific cases for any of the current list of special-case CSS properties.

jquery/test/unit/css.js

Lines 208 to 272 in 73d7e62

    
           QUnit.test( "css() non-px relative values (gh-1711)", function( assert ) { 
        
           	assert.expect( 17 ); 
        
           	var cssCurrent, 
        
           		units = {}, 
        
           		$child = jQuery( "#nothiddendivchild" ), 
        
           		add = function( prop, val, unit ) { 
        
           			var difference, 
        
           				adjustment = ( val < 0 ? "-=" : "+=" ) + Math.abs( val ) + unit, 
        
           				message = prop + ": " + adjustment, 
        
           				cssOld = cssCurrent, 
        
           				expected = cssOld + val * units[ prop ][ unit ]; 
        
           			// Apply change 
        
           			$child.css( prop, adjustment ); 
        
           			cssCurrent = parseFloat( $child.css( prop ) ); 
        
           			message += " (actual " + round( cssCurrent, 2 ) + "px, expected " + 
        
           				round( expected, 2 ) + "px)"; 
        
           			// Require a difference of no more than one pixel 
        
           			difference = Math.abs( cssCurrent - expected ); 
        
           			assert.ok( difference <= 1, message ); 
        
           		}, 
        
           		getUnits = function( prop ) { 
        
           			units[ prop ] = { 
        
           				"px": 1, 
        
           				"em": parseFloat( $child.css( prop, "100em" ).css( prop ) ) / 100, 
        
           				"pt": parseFloat( $child.css( prop, "100pt" ).css( prop ) ) / 100, 
        
           				"pc": parseFloat( $child.css( prop, "100pc" ).css( prop ) ) / 100, 
        
           				"cm": parseFloat( $child.css( prop, "100cm" ).css( prop ) ) / 100, 
        
           				"mm": parseFloat( $child.css( prop, "100mm" ).css( prop ) ) / 100, 
        
           				"%": parseFloat( $child.css( prop, "500%"  ).css( prop ) ) / 500 
        
           			}; 
        
           		}, 
        
           		round = function( num, fractionDigits ) { 
        
           			var base = Math.pow( 10, fractionDigits ); 
        
           			return Math.round( num * base ) / base; 
        
           		}; 
        
           	jQuery( "#nothiddendiv" ).css( { height: 1, padding: 0, width: 400 } ); 
        
           	$child.css( { height: 1, padding: 0 } ); 
        
           	getUnits( "width" ); 
        
           	cssCurrent = parseFloat( $child.css( "width", "50%" ).css( "width" ) ); 
        
           	add( "width",  25,    "%" ); 
        
           	add( "width", -50,    "%" ); 
        
           	add( "width",  10,   "em" ); 
        
           	add( "width",  10,   "pt" ); 
        
           	add( "width",  -2.3, "pt" ); 
        
           	add( "width",   5,   "pc" ); 
        
           	add( "width",  -5,   "em" ); 
        
           	add( "width",  +2,   "cm" ); 
        
           	add( "width", -15,   "mm" ); 
        
           	add( "width",  21,   "px" ); 
        
           	getUnits( "lineHeight" ); 
        
           	cssCurrent = parseFloat( $child.css( "lineHeight", "1em" ).css( "lineHeight" ) ); 
        
           	add( "lineHeight",  50,  "%" ); 
        
           	add( "lineHeight",   2, "em" ); 
        
           	add( "lineHeight", -10, "px" ); 
        
           	add( "lineHeight",  20, "pt" ); 
        
           	add( "lineHeight",  30, "pc" ); 
        
           	add( "lineHeight",   1, "cm" ); 
        
           	add( "lineHeight", -44, "mm" ); 
        
           } );

mgol · 2018-03-29T08:45:26Z

@enbo Could you submit a PR with your changes so that we can discuss them there?

The list looks good at first sight. As for the tests, we do have a few of assertions, see:

jquery/test/unit/css.js

Lines 1120 to 1144 in 73d7e62

    
           QUnit.test( "Do not append px (#9548, #12990, #2792)", function( assert ) { 
        
           	assert.expect( 3 ); 
        
           	var $div = jQuery( "<div>" ).appendTo( "#qunit-fixture" ); 
        
           	$div.css( "fill-opacity", 1 ); 
        
           	assert.equal( $div.css( "fill-opacity" ), 1, "Do not append px to 'fill-opacity'" ); 
        
           	$div.css( "column-count", 1 ); 
        
           	if ( $div.css( "column-count" ) !== undefined ) { 
        
           		assert.equal( $div.css( "column-count" ), 1, "Do not append px to 'column-count'" ); 
        
           	} else { 
        
           		assert.ok( true, "No support for column-count CSS property" ); 
        
           	} 
        
           	$div.css( "animation-iteration-count", 2 ); 
        
           	if ( $div.css( "animation-iteration-count" ) !== undefined ) { 
        
           		// if $div.css( "animation-iteration-count" ) return "1", 
        
           		// it actually return the default value of animation-iteration-count 
        
           		assert.equal( $div.css( "animation-iteration-count" ), 2, "Do not append px to 'animation-iteration-count'" ); 
        
           	} else { 
        
           		assert.ok( true, "No support for animation-iteration-count CSS property" ); 
        
           	} 
        
           } );

You could add the grid ones to the test. You'll need to check for grid support first, similarly to how animation-iteration-count is treated. I think you may just check if $div.css( "display", "grid" ) is set properly and put all the assertions inside this if's then block.

mgol · 2018-03-29T19:01:54Z

You accidentally closed it from a PR to your fork. :) Reopening.

This commit adds some CSS grid-related properties to jQuery.cssNumber. Fixes gh-4007

Ref jquerygh-4007 Ref jquerygh-4028

Ref gh-4007 Ref gh-4028 Closes gh-4165

mgol added Bug CSS labels Mar 13, 2018

mgol added this to the 3.4.0 milestone Mar 13, 2018

enbo referenced this issue in enbo/jquery Mar 28, 2018

Add possibly-unitless CSS grid properties

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

078ed8d

enbo mentioned this issue Mar 29, 2018

Add possibly-unitless CSS grid properties enbo/jquery#1

Merged

1 of 4 tasks complete

enbo closed this in enbo/jquery#1 Mar 29, 2018

enbo mentioned this issue Mar 29, 2018

Add possibly-unitless CSS grid properties #4028

Merged

1 of 4 tasks complete

mgol reopened this Mar 29, 2018

mgol added the Has Pull Request label Mar 29, 2018

mgol closed this in #4028 Aug 29, 2018

mgol added a commit that referenced this issue Aug 29, 2018

CSS: Don't auto-append "px" to possibly-unitless CSS grid properties

Loading status checks…

f997241

This commit adds some CSS grid-related properties to jQuery.cssNumber. Fixes gh-4007

mgol added a commit to mgol/jquery that referenced this issue Aug 29, 2018

Tests: Add tests for not auto-appending "px" to CSS Grid properties

af78b60

Ref jquerygh-4007 Ref jquerygh-4028

mgol mentioned this issue Aug 29, 2018

Tests: Add tests for not auto-appending "px" to CSS Grid properties #4165

Merged

3 of 3 tasks complete

mgol added a commit to mgol/jquery that referenced this issue Oct 3, 2018

Tests: Add tests for not auto-appending "px" to CSS Grid properties

0dd32d3

Ref jquerygh-4007 Ref jquerygh-4028

GulajavaMinistudio mentioned this issue Oct 3, 2018

The compiler reported twice GulajavaMinistudio/jquery#61

Merged

0 of 4 tasks complete

lock bot locked as resolved and limited conversation to collaborators Feb 25, 2019

jquery / jquery

Cannot set property "grid-column-start" using .css() #4007

Cannot set property "grid-column-start" using .css() #4007

enbo commented Mar 13, 2018 •

edited

mgol commented Mar 13, 2018 •

edited

mgol commented Mar 13, 2018

enbo commented Mar 13, 2018

enbo commented Mar 13, 2018

mgol commented Mar 13, 2018

mgol commented Mar 13, 2018

enbo commented Mar 28, 2018

mgol commented Mar 29, 2018

mgol commented Mar 29, 2018

jquery / jquery

Cannot set property "grid-column-start" using .css() #4007

Cannot set property "grid-column-start" using .css() #4007

Comments

enbo commented Mar 13, 2018 • edited

Description

Link to test case

mgol commented Mar 13, 2018 • edited

mgol commented Mar 13, 2018

enbo commented Mar 13, 2018

enbo commented Mar 13, 2018

mgol commented Mar 13, 2018

mgol commented Mar 13, 2018

enbo commented Mar 28, 2018

mgol commented Mar 29, 2018

mgol commented Mar 29, 2018

enbo commented Mar 13, 2018 •

edited

mgol commented Mar 13, 2018 •

edited