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.

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

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

Closed
enbo opened this Issue Mar 13, 2018 · 9 comments

Comments

Projects
None yet
2 participants
@enbo
Copy link
Contributor

commented Mar 13, 2018

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

JSFiddle

@mgol mgol added Bug CSS labels Mar 13, 2018

@mgol mgol added this to the 3.4.0 milestone Mar 13, 2018

@mgol

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

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

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2018

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

@mgol

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

@mgol

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

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

@enbo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2018

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

This comment has been minimized.

Copy link
Member

commented Mar 29, 2018

@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.

@enbo enbo referenced this issue Mar 29, 2018

Merged

Add possibly-unitless CSS grid properties #1

1 of 4 tasks complete

@enbo enbo referenced this issue Mar 29, 2018

Merged

Add possibly-unitless CSS grid properties #4028

1 of 4 tasks complete
@mgol

This comment has been minimized.

Copy link
Member

commented Mar 29, 2018

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

@mgol mgol reopened this Mar 29, 2018

@mgol 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
This commit adds some CSS grid-related properties to jQuery.cssNumber.

Fixes gh-4007

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

@GulajavaMinistudio GulajavaMinistudio referenced this issue Oct 3, 2018

Merged

The compiler reported twice #61

0 of 4 tasks complete

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.