Skip to content

CSS: Correct misrepresentation of "auto" horizontal margins as 0 #2276

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

Closed
gibson042 wants to merge 2 commits into from
Closed

CSS: Correct misrepresentation of "auto" horizontal margins as 0 #2276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
866ef5f
CSS: Correct misrepresentation of "auto" horizontal margins as 0
gibson042 May 8, 2015
2fc49f6
CSS: Only perform expensive marginLeft calculation when necessary
gibson042 May 8, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/css.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,19 @@ jQuery.cssHooks.marginRight = addGetHookIf( support.reliableMarginRight,
}
);

jQuery.cssHooks.marginLeft = addGetHookIf( support.reliableMarginLeft,
function( elem, computed ) {
if ( computed ) {
return ( parseFloat( curCSS( elem, "marginLeft" ) ) ||
elem.getBoundingClientRect().left -
swap( elem, { marginLeft: 0 }, function() {
return elem.getBoundingClientRect().left;
} )
) + "px";
}
}
);

// These hooks are used by animate to expand properties
jQuery.each({
margin: "",
Expand Down
30 changes: 21 additions & 9 deletions src/css/support.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ define([

(function() {
var div, container, style, a, pixelPositionVal, boxSizingReliableVal, pixelMarginRightVal,
reliableHiddenOffsetsVal, reliableMarginRightVal;
reliableHiddenOffsetsVal, reliableMarginRightVal, reliableMarginLeftVal;

// Setup
div = document.createElement( "div" );
Expand Down Expand Up @@ -56,7 +56,7 @@ define([
},

pixelMarginRight: function() {
// Support: Android 4.0-4.3
// Support: Android 4.0 - 4.3 only
if ( pixelPositionVal == null ) {
computeStyleTests();
}
Expand All @@ -76,6 +76,14 @@ define([
computeStyleTests();
}
return reliableMarginRightVal;
},

reliableMarginLeft: function() {
// Support: IE <=8 only, Android 4.0 - 4.3 only, Firefox <=3 - 37
Copy link
Member

@markelog markelog May 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, there is no spaces in ranges :-), i.e. Android 4.0-4.3 only

Copy link
Member Author

@gibson042 gibson042 May 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jquery/contribute.jquery.org/issues/95

Copy link
Member

@markelog markelog May 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it seems we didn't had consensus about this issue, it did come down to like/don't like, i'd say we should follow some standart here, if we so nit-picky :-).

http://practicaltypography.com/hyphens-and-dashes.html
http://en.wikipedia.org/wiki/Dash#Ranges_of_values

if ( pixelPositionVal == null ) {
computeStyleTests();
}
return reliableMarginLeftVal;
}
});

Expand All @@ -95,24 +103,28 @@ define([
// Support: Android 2.3
// Vendor-prefix box-sizing
"-webkit-box-sizing:border-box;box-sizing:border-box;" +
"position:absolute;display:block;" +
"margin:0;margin-top:1%;margin-right:50%;" +
"border:1px;padding:1px;" +
"top:1%;height:4px;width:50%";
"position:relative;display:block;" +
"margin:auto;border:1px;padding:1px;" +
"top:1%;width:50%";

// Support: IE<9
// Assume reasonable values in the absence of getComputedStyle
pixelPositionVal = boxSizingReliableVal = false;
pixelPositionVal = boxSizingReliableVal = reliableMarginLeftVal = false;
pixelMarginRightVal = reliableMarginRightVal = true;

// Check for getComputedStyle so that this code is not run in IE<9.
if ( window.getComputedStyle ) {
divStyle = window.getComputedStyle( div, null );
pixelPositionVal = ( divStyle || {} ).top !== "1%";
boxSizingReliableVal = ( divStyle || { height: "4px" } ).height === "4px";
reliableMarginLeftVal = ( divStyle || {} ).marginLeft === "2px";
boxSizingReliableVal = ( divStyle || { width: "4px" } ).width === "4px";

// Support: Android 4.0 - 4.3 only
// Some styles come back with percentage values, even though they shouldn't
div.style.marginRight = "50%";
pixelMarginRightVal = ( divStyle || { marginRight: "4px" } ).marginRight === "4px";

// Support: Android 2.3
// Support: Android 2.3 only
// Div with explicit width and no margin-right incorrectly
// gets computed margin-right based on width of container (#3333)
// WebKit Bug 13343 - getComputedStyle returns wrong value for margin-right
Expand Down
3 changes: 2 additions & 1 deletion test/data/offset/relative.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
body { margin: 1px; padding: 5px; }
div.relative { position: relative; top: 0; left: 0; margin: 1px; border: 2px solid #000; padding: 5px; width: 100px; height: 100px; background: #fff; overflow: hidden; }
#relative-2 { top: 20px; left: 20px; }
#relative-2-1 { margin: auto; width: 50px; }
#marker { position: absolute; border: 2px solid #000; width: 50px; height: 50px; background: #ccc; }
</style>
<script src="../../jquery.js"></script>
Expand All @@ -24,7 +25,7 @@
</head>
<body>
<div id="relative-1" class="relative"><div id="relative-1-1" class="relative"><div id="relative-1-1-1" class="relative"></div></div></div>
<div id="relative-2" class="relative"></div>
<div id="relative-2" class="relative"><div id="relative-2-1" class="relative"></div></div>
<div id="marker"></div>
<p class="instructions">Click the white box to move the marker to it. Clicking the box also changes the position to absolute (if not already) and sets the position according to the position method.</p>
</body>
Expand Down
23 changes: 19 additions & 4 deletions test/unit/css.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -788,16 +788,31 @@ test("internal ref to elem.runtimeStyle (bug #7608)", function () {
ok( result, "elem.runtimeStyle does not throw exception" );
});

test("marginRight computed style (bug #3333)", function() {
expect(1);
test("computed margins (trac-3333; gh-2237)", function() {
expect( 2 );

var $div = jQuery( "#foo" ),
$child = jQuery( "#en" );

var $div = jQuery("#foo");
$div.css({
"width": "1px",
"marginRight": 0
});
equal( $div.css( "marginRight" ), "0px",
"marginRight correctly calculated with a width and display block" );

equal($div.css("marginRight"), "0px", "marginRight correctly calculated with a width and display block");
$div.css({
position: "absolute",
top: 0,
left: 0,
width: "100px"
});
$child.css({
width: "50px",
margin: "auto"
});
equal( $child.css( "marginLeft" ), "25px",
"auto margins are computed to pixels" );
});

test("box model properties incorrectly returning % instead of px, see #10639 and #12088", function() {
Expand Down
8 changes: 5 additions & 3 deletions test/unit/offset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,15 +180,16 @@ testIframe("offset/absolute", "absolute", function( $ ) {
});

testIframe("offset/relative", "relative", function( $ ) {
expect(60);
expect( 64 );

var tests;

// get offset
tests = [
{ "id": "#relative-1", "top": 7, "left": 7 },
{ "id": "#relative-1-1", "top": 15, "left": 15 },
{ "id": "#relative-2", "top": 142, "left": 27 }
{ "id": "#relative-2", "top": 142, "left": 27 },
{ "id": "#relative-2-1", "top": 149, "left": 52 }
];
jQuery.each( tests, function() {
equal( $( this["id"] ).offset().top, this["top"], "jQuery('" + this["id"] + "').offset().top" );
Expand All @@ -200,7 +201,8 @@ testIframe("offset/relative", "relative", function( $ ) {
tests = [
{ "id": "#relative-1", "top": 6, "left": 6 },
{ "id": "#relative-1-1", "top": 5, "left": 5 },
{ "id": "#relative-2", "top": 141, "left": 26 }
{ "id": "#relative-2", "top": 141, "left": 26 },
{ "id": "#relative-2-1", "top": 5, "left": 5 }
];
jQuery.each( tests, function() {
equal( $( this["id"] ).position().top, this["top"], "jQuery('" + this["id"] + "').position().top" );
Expand Down
50 changes: 32 additions & 18 deletions test/unit/support.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ testIframeWithCallback( "Check CSP (https://developer.mozilla.org/en-US/docs/Sec
"radioValue": true,
"reliableHiddenOffsets": true,
"reliableMarginRight": true,
"reliableMarginLeft": true,
"style": true,
"submitBubbles": true
};
Expand Down Expand Up @@ -131,6 +132,7 @@ testIframeWithCallback( "Check CSP (https://developer.mozilla.org/en-US/docs/Sec
"radioValue": false,
"reliableHiddenOffsets": true,
"reliableMarginRight": true,
"reliableMarginLeft": true,
"style": true,
"submitBubbles": true
};
Expand Down Expand Up @@ -163,6 +165,7 @@ testIframeWithCallback( "Check CSP (https://developer.mozilla.org/en-US/docs/Sec
"radioValue": false,
"reliableHiddenOffsets": true,
"reliableMarginRight": true,
"reliableMarginLeft": true,
"style": true,
"submitBubbles": true
};
Expand Down Expand Up @@ -195,6 +198,7 @@ testIframeWithCallback( "Check CSP (https://developer.mozilla.org/en-US/docs/Sec
"radioValue": false,
"reliableHiddenOffsets": false,
"reliableMarginRight": true,
"reliableMarginLeft": false,
"style": false,
"submitBubbles": false
};
Expand Down Expand Up @@ -227,6 +231,7 @@ testIframeWithCallback( "Check CSP (https://developer.mozilla.org/en-US/docs/Sec
"radioValue": true,
"reliableHiddenOffsets": true,
"reliableMarginRight": true,
"reliableMarginLeft": true,
"style": true,
"submitBubbles": true
};
Expand Down Expand Up @@ -259,6 +264,7 @@ testIframeWithCallback( "Check CSP (https://developer.mozilla.org/en-US/docs/Sec
"radioValue": true,
"reliableHiddenOffsets": true,
"reliableMarginRight": true,
"reliableMarginLeft": true,
"style": true,
"submitBubbles": true
};
Expand Down Expand Up @@ -291,6 +297,7 @@ testIframeWithCallback( "Check CSP (https://developer.mozilla.org/en-US/docs/Sec
"radioValue": true,
"reliableHiddenOffsets": true,
"reliableMarginRight": true,
"reliableMarginLeft": false,
"style": true,
"submitBubbles": true
};
Expand Down Expand Up @@ -323,6 +330,7 @@ testIframeWithCallback( "Check CSP (https://developer.mozilla.org/en-US/docs/Sec
"radioValue": true,
"reliableHiddenOffsets": true,
"reliableMarginRight": true,
"reliableMarginLeft": true,
"style": true,
"submitBubbles": true
};
Expand Down Expand Up @@ -355,6 +363,7 @@ testIframeWithCallback( "Check CSP (https://developer.mozilla.org/en-US/docs/Sec
"radioValue": true,
"reliableHiddenOffsets": true,
"reliableMarginRight": true,
"reliableMarginLeft": true,
"style": true,
"submitBubbles": true
};
Expand Down Expand Up @@ -387,6 +396,7 @@ testIframeWithCallback( "Check CSP (https://developer.mozilla.org/en-US/docs/Sec
"radioValue": true,
"reliableHiddenOffsets": true,
"reliableMarginRight": true,
"reliableMarginLeft": false,
"style": true,
"submitBubbles": true
};
Expand Down Expand Up @@ -419,32 +429,36 @@ testIframeWithCallback( "Check CSP (https://developer.mozilla.org/en-US/docs/Sec
"radioValue": true,
"reliableHiddenOffsets": true,
"reliableMarginRight": false,
"reliableMarginLeft": true,
"style": true,
"submitBubbles": true
};
}

if ( expected ) {
test( "Verify that the support tests resolve as expected per browser", function() {
var i, prop,
j = 0;
test( "Verify that the support tests resolve as expected per browser", function() {
if ( !expected ) {
expect( 1 );
ok( false, "Known client: " + userAgent );
}

for ( prop in computedSupport ) {
j++;
}
var i, prop,
j = 0;

for ( prop in computedSupport ) {
j++;
}

expect( j );
expect( j );

for ( i in expected ) {
if ( jQuery.ajax || i !== "ajax" && i !== "cors" ) {
equal( computedSupport[i], expected[i],
"jQuery.support['" + i + "']: " + computedSupport[i] +
", expected['" + i + "']: " + expected[i]);
} else {
ok( true, "no ajax; skipping jQuery.support['" + i + "']" );
}
for ( i in expected ) {
if ( jQuery.ajax || i !== "ajax" && i !== "cors" ) {
equal( computedSupport[i], expected[i],
"jQuery.support['" + i + "']: " + computedSupport[i] +
", expected['" + i + "']: " + expected[i]);
} else {
ok( true, "no ajax; skipping jQuery.support['" + i + "']" );
}
});
}
}
});

})();