The Wayback Machine - https://web.archive.org/web/20150323053651/https://github.com/jquery/jquery/pull/1241/files
Skip to content
Sign up Sign in
This repository

/jquery

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Make left/top auto value consistent across browsers #1241

Closed
wants to merge 8 commits into from
+27 −6

Labels

None yet

Milestone

  3.0.0

Assignee

No one assigned

3 participants

@yiminghe @mzgol @dmethvin
@yiminghe
yiminghe commented

http://bugs.jquery.com/ticket/13767

tests:

http://jsfiddle.net/yiminghe/Yu9sP/5/

@yiminghe yiminghe referenced this pull request
Closed

retrieve css left value with px unit #1240

@mzgol
Collaborator
mzgol commented

You don't have to create a new pull request when you change something; if you push your changes to the same branch, your pull request is automatically updated.

Please rebase your changes to the current master branch (or merge the current master branch to yours if that's more comfortable for you) since currently there are conflicts making merge impossible.

Also, please correct your changes according to the style guide: http://contribute.jquery.org/style-guide/js/. I'll have further remarks later.

Thanks!

@yiminghe
yiminghe commented

@mzgol

I am sorry for the code-style problem because my IDE is configed according to my project's code style rules.
As for the new pr, I just want to have a better branch name for you to rebase/merge.
I have rewritten the patch based on current master and jquery code style, please review.

Thanks.

@mzgol Show outdated diff Hide outdated diff mzgol commented on an outdated diff
src/css.js
@@ -506,9 +506,19 @@ jQuery(function() {
jQuery.cssHooks[ prop ] = {
get: function( elem, computed ) {
if ( computed ) {
+ var el = jQuery(elem),
+ position = el.css("position"),
+ isAutoPosition;
@mzgol Collaborator
mzgol added a note

This should be in the first line, see:
http://contribute.jquery.org/style-guide/js/
point 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mzgol Show outdated diff Hide outdated diff mzgol commented on an outdated diff
src/css.js
@@ -506,9 +506,19 @@ jQuery(function() {
jQuery.cssHooks[ prop ] = {
get: function( elem, computed ) {
if ( computed ) {
+ var el = jQuery(elem),
+ position = el.css("position"),
+ isAutoPosition;
+ if( position === "static" ) {
@mzgol Collaborator
mzgol added a note

Space after if, the same in other places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mzgol Show outdated diff Hide outdated diff mzgol commented on an outdated diff
src/css.js
((10 lines not shown))
computed = curCSS( elem, prop );
- // if curCSS returns percentage, fallback to offset
- return rnumnonpx.test( computed ) ?
+ isAutoPosition = computed === "auto";
+ if( isAutoPosition && position === "relative" ) {
@mzgol Collaborator
mzgol added a note

I'm not sure if this case really needs to be separated. @mikesherov, what do you think? (also, space after if missing)

@mzgol Collaborator
mzgol added a note

Actually, I'm quite sure it's not needed, especially that you don't take into account 1% and similar things and it just adds bytes. Fallback to position() is enough.

@mzgol Collaborator
mzgol added a note

Also, if you remove this condition, the isAutoPosition variable gets redundant, you can paste the condition directly below.

@yiminghe
yiminghe added a note

relative element with left/top auto needs to return 0px immediately and can not get px value by offsetParent/offsetTop/offsetLeft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mzgol Show outdated diff Hide outdated diff mzgol commented on an outdated diff
src/css.js
@@ -506,9 +506,19 @@ jQuery(function() {
jQuery.cssHooks[ prop ] = {
get: function( elem, computed ) {
if ( computed ) {
+ var el = jQuery(elem),
+ position = el.css("position"),
+ isAutoPosition;
+ if( position === "static" ) {
+ return "auto";
@mzgol Collaborator
mzgol added a note

not "auto", just what curCSS( elem, prop ) returns, i.e. the computed variable from below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mzgol Show outdated diff Hide outdated diff mzgol commented on an outdated diff
src/css.js
@@ -506,9 +506,19 @@ jQuery(function() {
jQuery.cssHooks[ prop ] = {
get: function( elem, computed ) {
if ( computed ) {
+ var el = jQuery(elem),
+ position = el.css("position"),
@mzgol Collaborator
mzgol added a note

We use curCSS( elem, prop ) internally, it's faster. Also, current code causes invoking getStyles( elem ) twice (see the beginning of curCSS) which adds overhead. curCSS accepts the third optional parameter _computed, you should use styles = getStyles( elem ) once and pass the result here and also change the line:

computed = curCSS( elem, prop );

to:

computed = curCSS( elem, prop, styles );
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mzgol mzgol commented on the diff
View full changes
src/support.js
@@ -89,8 +89,9 @@ jQuery.support = (function( support ) {
// Use window.getComputedStyle because jsdom on node.js will break without it.
if ( window.getComputedStyle ) {
- support.pixelPosition = ( window.getComputedStyle( div, null ) || {} ).top !== "1%";
- support.boxSizingReliable = ( window.getComputedStyle( div, null ) || { width: "4px" } ).width === "4px";
+ divStyle = window.getComputedStyle( div, null ) || { width: "4px" };
@mzgol Collaborator
mzgol added a note

This adds 5 bytes but prevents getComputedStyle to be invoked twice which is nice. @mikesherov, what's your opinion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mzgol Show outdated diff Hide outdated diff mzgol commented on an outdated diff
test/unit/css.js
@@ -874,6 +874,19 @@ test( "css opacity consistency across browsers (#12685)", function() {
equal( Math.round( el.css("opacity") * 100 ), 20, "remove opacity override" );
});
+test( "css left/top auto consistency across browsers (#13767)", function() {
+ expect( 4 );
+
+ var fixture = jQuery("#qunit-fixture"),
+ el = jQuery("<div style='position: relative;padding: 20px;'>" +
+ "<div style='position: absolute'></div><span></span><s style='position: fixed'></s></div>").appendTo(fixture);
+
+ equal( el.css("left"), "0px" );
+ equal( el.find("div").css("top"), "20px" );
+ equal( el.find("span").css("top"), "auto" );
+ equal( el.find("s").css("top").indexOf("px") != -1, true );
@mzgol Collaborator
mzgol added a note

use notEqual for this last assertion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mzgol Show outdated diff Hide outdated diff mzgol commented on an outdated diff
src/css.js
@@ -506,9 +506,19 @@ jQuery(function() {
jQuery.cssHooks[ prop ] = {
get: function( elem, computed ) {
if ( computed ) {
- computed = curCSS( elem, prop );
- // if curCSS returns percentage, fallback to offset
- return rnumnonpx.test( computed ) ?
+ var isAutoPosition,
+ elStyles = getStyles( elem ),
+ position = curCSS( elem, "position", elStyles );
+ if ( position === "static" ) {
+ return "auto";
@mzgol Collaborator
mzgol added a note

Still: not "auto", just what curCSS( elem, prop, elStyles ) returns, i.e. the computed variable from below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mzgol Show outdated diff Hide outdated diff mzgol commented on an outdated diff
src/css.js
@@ -506,9 +506,19 @@ jQuery(function() {
jQuery.cssHooks[ prop ] = {
get: function( elem, computed ) {
if ( computed ) {
- computed = curCSS( elem, prop );
- // if curCSS returns percentage, fallback to offset
- return rnumnonpx.test( computed ) ?
+ var isAutoPosition,
+ elStyles = getStyles( elem ),
+ position = curCSS( elem, "position", elStyles );
+ if ( position === "static" ) {
+ return "auto";
+ }
+ computed = curCSS( elem, prop, elStyles );
+ isAutoPosition = computed === "auto";
+ if ( isAutoPosition && position === "relative" ) {
@mzgol Collaborator
mzgol added a note

You're right this is needed... It's a lot of bytes, though, I wonder if there's a shorter way to achieve that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mzgol Show outdated diff Hide outdated diff mzgol commented on an outdated diff
src/css.js
@@ -506,9 +506,19 @@ jQuery(function() {
jQuery.cssHooks[ prop ] = {
get: function( elem, computed ) {
if ( computed ) {
- computed = curCSS( elem, prop );
- // if curCSS returns percentage, fallback to offset
- return rnumnonpx.test( computed ) ?
+ var isAutoPosition,
+ elStyles = getStyles( elem ),
+ position = curCSS( elem, "position", elStyles );
+ if ( position === "static" ) {
@mzgol Collaborator
mzgol added a note

BTW, since we want computed to be returned here, you can move this check to line 521, just after isAutoPosition check.

@yiminghe
yiminghe added a note

ok, but it will cause extra code to run at runtime for static case.
since getting left/top of position static element is rare condition, it's fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dmethvin
Owner
dmethvin commented

Where are we with this pull request? @yiminghe is it okay? @mikesherov any feedback on the question posed above?

@mzgol
Collaborator
mzgol commented

@dmethvin IIRC @mikesherov agreed it's a good fix but don't take my word for it. :)

@yiminghe
yiminghe commented

I had adjust code by request a month ago, it should be ok.

@mzgol
Collaborator
mzgol commented

Updated test case: http://jsfiddle.net/QzvH5/15/

@mzgol
Collaborator
mzgol commented

IMO it's good to merge. Any final thoughts about that, @dmethvin?

This fix adds 35 bytes to the minified gzipped copy (and another 5 due to getComputedStyle optimization but that's a separate issue) but I'm afraid that's unavoidable in this case...

@mzgol
Collaborator
mzgol commented

We'll wait for 2.1/1.11 with that.

@dmethvin dmethvin added this to the 1.12/2.2 milestone
@MarcDiethelm MarcDiethelm referenced this pull request in MarcDiethelm/contributing
Open

Integrate jQuery PR wisdom #1

@dmethvin
Owner
dmethvin commented

We closed the original ticket wontfix and haven't heard any wailing, so I'll close this.

@dmethvin dmethvin closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 15, 2013
  1. yiminghe

    css left/top auto consistency across browsers. Fixes #13767

    yiminghe authored
    e4c2107
Commits on Apr 16, 2013
  1. yiminghe

    adjust code style

    yiminghe authored
    d3579a3
  2. yiminghe

    adjust code style

    yiminghe authored
    e80e958
  3. yiminghe

    adjust code style

    yiminghe authored
    2d010d3
Commits on Apr 18, 2013
  1. yiminghe

    return auto instead of 0px

    yiminghe authored
    23ef94e
  2. yiminghe

    remove extra variable

    yiminghe authored
    0bb23dd
  3. yiminghe

    revert to return 0px

    yiminghe authored
    e158984
  4. yiminghe

    remove position static if condition

    yiminghe authored
    c1e856f
This page is out of date. Refresh to see the latest.
Unified Split
Showing with 27 additions and 6 deletions.
  1. +10 −3 src/css.js
  2. +4 −3 src/support.js
  3. +13 −0 test/unit/css.js
View
13 src/css.js
@@ -506,9 +506,16 @@ jQuery(function() {
jQuery.cssHooks[ prop ] = {
get: function( elem, computed ) {
if ( computed ) {
- computed = curCSS( elem, prop );
- // if curCSS returns percentage, fallback to offset
- return rnumnonpx.test( computed ) ?
+ var isAutoPosition,
+ elStyles = getStyles( elem ),
+ position = curCSS( elem, "position", elStyles );
+ computed = curCSS( elem, prop, elStyles );
+ isAutoPosition = computed === "auto";
+ if ( isAutoPosition && position === "relative" ) {
+ return "0px";
+ }
+ // if curCSS returns percentage or auto, fallback to offset
+ return isAutoPosition && position !== "static" || rnumnonpx.test( computed ) ?
jQuery( elem ).position()[ prop ] + "px" :
computed;
}
View
7 src/support.js
@@ -62,7 +62,7 @@ jQuery.support = (function( support ) {
// Run tests that need a body at doc ready
jQuery(function() {
- var container, marginDiv,
+ var container, marginDiv, divStyle,
// Support: Firefox, Android 2.3 (Prefixed box-sizing versions).
divReset = "padding:0;margin:0;border:0;display:block;-webkit-box-sizing:content-box;-moz-box-sizing:content-box;box-sizing:content-box",
body = document.getElementsByTagName("body")[ 0 ];
@@ -89,8 +89,9 @@ jQuery.support = (function( support ) {
// Use window.getComputedStyle because jsdom on node.js will break without it.
if ( window.getComputedStyle ) {
- support.pixelPosition = ( window.getComputedStyle( div, null ) || {} ).top !== "1%";
- support.boxSizingReliable = ( window.getComputedStyle( div, null ) || { width: "4px" } ).width === "4px";
+ divStyle = window.getComputedStyle( div, null ) || { width: "4px" };
@mzgol Collaborator
mzgol added a note

This adds 5 bytes but prevents getComputedStyle to be invoked twice which is nice. @mikesherov, what's your opinion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ support.pixelPosition = divStyle.top !== "1%" && divStyle.left !== "auto";
+ support.boxSizingReliable = divStyle.width === "4px";
// Support: Android 2.3
// Check if div with explicit width and no margin-right incorrectly
View
13 test/unit/css.js
@@ -874,6 +874,19 @@ test( "css opacity consistency across browsers (#12685)", function() {
equal( Math.round( el.css("opacity") * 100 ), 20, "remove opacity override" );
});
+test( "css left/top auto consistency across browsers (#13767)", function() {
+ expect( 4 );
+
+ var fixture = jQuery("#qunit-fixture"),
+ el = jQuery("<div style='position: relative;padding: 20px;'>" +
+ "<div style='position: absolute'></div><span></span><s style='position: fixed'></s></div>").appendTo(fixture);
+
+ equal( el.css("left"), "0px" );
+ equal( el.find("div").css("top"), "20px" );
+ equal( el.find("span").css("top"), "auto" );
+ notEqual( el.find("s").css("top").indexOf("px"), -1 );
+});
+
test( ":visible/:hidden selectors", function() {
expect( 13 );
Something went wrong with that request. Please try again.
Morty Proxy This is a proxified and sanitized view of the page, visit original site.