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

width(), height(), etc. shouldn't round #1724

Closed
mgol opened this issue Oct 20, 2014 · 25 comments
Closed

width(), height(), etc. shouldn't round #1724

mgol opened this issue Oct 20, 2014 · 25 comments

Comments

@mgol
Copy link
Member

mgol commented Oct 20, 2014

Originally reported by john at: http://bugs.jquery.com/ticket/9628

Right now we're rounding off the answers from width(), height(), etc. to the nearest pixel - this makes it hard to do good positioning of elements (especially for animations). For example if you have 3 items that are width 33% inside of a 100px element we're returning 33px for each element. Theoretically an animation method might be able to return a more interesting result if they know that there are extra fractional-numbers getting rounded off.

I got a report on this directly from Mozilla but I agree with them that this particular change makes it hard to get to good numbers.

See also: #3239.

Issue reported for jQuery 1.6.1

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: rwaldron

For normalization purposes, jQuery core will maintain round pixel value returns, however you can easily write a cssHook to return the width/height in any value format that you prefer.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: bzbarsky

Just to be clear, this bug makes width() completely useless for layout calculations in UAs that do subpixel layout: you compute some widths that look like they should fit in a container, but in fact they don't.

People are running into this all the time in UAs that in fact do subpixel layout (e.g. not WebKit).

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: mikesherov

Boris, what do you suggest here, to just stop rounding if a fraction is returned?

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: dmethvin

#12870 is a duplicate of this ticket.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: mikesherov

#12373 is a duplicate of this ticket.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: mikesherov

bzbarsky, if we could get FF to return floats for offsetWIdth and offsetHeight, that would solve this is. In fact, jQuery doesn't use the getComputedStyle().height for calculations. It uses offsetHeight, then subtracts/adds padding, border, margin as necessary. Perhaps I can file a bug with you about it ;-)

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: mikesherov

bzbarsky, reported to FF here:  https://bugzilla.mozilla.org/show_bug.cgi?id=825607

I might take a stab at ditching offsetWidth/offsetHeight for jQuery 2.0 once oldIE codepaths are gone, but this would be fixed automatically if offsetWidth/Height stopped rounding.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: mikesherov

From Boris:  https://bugzilla.mozilla.org/show_bug.cgi?id=825607#c5

Just don't use offset*. Use getBoundingClientRect."

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: mikesherov

SO, the good news is that for FF, Chrome, Safari, and Opera , getBoundingClientRect work great, reporting subpixel values when appropriate, and containing a "width" property:

 http://jsfiddle.net/7MMhB/4/

However, the bad news is, IE9 reports "2" (as opposed to 3 for offsetWidth), IE<9 reports a TextRectangle object with no width property. Sigh.

If we truly want to get all subpixely, either we now see if we can drop offsetWidth altogether, or we do a feature detect for subpixely getBoundingRectClient, and use that when we can.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: mikesherov

Oh, and IE10 works right too.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: dmethvin

#14039 is a duplicate of this ticket.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: dmethvin

#14556 is a duplicate of this ticket.

@mgol
Copy link
Member Author

mgol commented Oct 20, 2014

Author: dmethvin

Ref:  https://codereview.chromium.org/340903002/

Still, jQuery may be in a better position to do this, since the site gets to choose whether it wants to upgrade to a newer jQuery. It's a question of whether there are a lot of popular plugins that would break with fractional widths and heights.

@mgol mgol added the Bug label Oct 21, 2014
@dmethvin
Copy link
Member

I'm going to mark this for potential inclusion in the 3.0 release, it might be the best place to try a breaking change like this.

@dmethvin dmethvin added this to the 3.0.0 milestone Oct 24, 2014
@mgol mgol assigned mgol and unassigned mikesherov Oct 27, 2014
@mgol
Copy link
Member Author

mgol commented Oct 27, 2014

@mikesherov Assigning to myself but if you want to do it, please re-assign back (and notify me plz).

@MarventusWP
Copy link

I would like to add, since I don't see it mentioned anywhere in here, that .css("height") and .css("width") are also rounding values. Here's a fiddle: http://jsfiddle.net/Marventus/L16gmjuz/
Hope this helps.

@mgol
Copy link
Member Author

mgol commented Apr 27, 2015

I would like to add, since I don't see it mentioned anywhere in here, that .css("height") and .css("width") are also rounding values.

Yes, that's caused by the same code. .css('width') is going through a special hook instead of getComputedStyle so that it works on disconnected nodes, see:

jquery/src/css.js

Lines 353 to 382 in 002240a

jQuery.each([ "height", "width" ], function( i, name ) {
jQuery.cssHooks[ name ] = {
get: function( elem, computed, extra ) {
if ( computed ) {
// Certain elements can have dimension info if we invisibly show them
// but it must have a current display style that would benefit
return rdisplayswap.test( jQuery.css( elem, "display" ) ) &&
elem.offsetWidth === 0 ?
swap( elem, cssShow, function() {
return getWidthOrHeight( elem, name, extra );
}) :
getWidthOrHeight( elem, name, extra );
}
},
set: function( elem, value, extra ) {
var styles = extra && getStyles( elem );
return setPositiveNumber( elem, value, extra ?
augmentWidthOrHeight(
elem,
name,
extra,
jQuery.css( elem, "boxSizing", false, styles ) === "border-box",
styles
) : 0
);
}
};
});

and:

jquery/src/css.js

Lines 71 to 152 in 002240a

function augmentWidthOrHeight( elem, name, extra, isBorderBox, styles ) {
var i = extra === ( isBorderBox ? "border" : "content" ) ?
// If we already have the right measurement, avoid augmentation
4 :
// Otherwise initialize for horizontal or vertical properties
name === "width" ? 1 : 0,
val = 0;
for ( ; i < 4; i += 2 ) {
// Both box models exclude margin, so add it if we want it
if ( extra === "margin" ) {
val += jQuery.css( elem, extra + cssExpand[ i ], true, styles );
}
if ( isBorderBox ) {
// border-box includes padding, so remove it if we want content
if ( extra === "content" ) {
val -= jQuery.css( elem, "padding" + cssExpand[ i ], true, styles );
}
// At this point, extra isn't border nor margin, so remove border
if ( extra !== "margin" ) {
val -= jQuery.css( elem, "border" + cssExpand[ i ] + "Width", true, styles );
}
} else {
// At this point, extra isn't content, so add padding
val += jQuery.css( elem, "padding" + cssExpand[ i ], true, styles );
// At this point, extra isn't content nor padding, so add border
if ( extra !== "padding" ) {
val += jQuery.css( elem, "border" + cssExpand[ i ] + "Width", true, styles );
}
}
}
return val;
}
function getWidthOrHeight( elem, name, extra ) {
// Start with offset property, which is equivalent to the border-box value
var valueIsBorderBox = true,
val = name === "width" ? elem.offsetWidth : elem.offsetHeight,
styles = getStyles( elem ),
isBorderBox = jQuery.css( elem, "boxSizing", false, styles ) === "border-box";
// Some non-html elements return undefined for offsetWidth, so check for null/undefined
// svg - https://bugzilla.mozilla.org/show_bug.cgi?id=649285
// MathML - https://bugzilla.mozilla.org/show_bug.cgi?id=491668
if ( val <= 0 || val == null ) {
// Fall back to computed then uncomputed css if necessary
val = curCSS( elem, name, styles );
if ( val < 0 || val == null ) {
val = elem.style[ name ];
}
// Computed unit is not pixels. Stop here and return.
if ( rnumnonpx.test(val) ) {
return val;
}
// Check for style in case a browser which returns unreliable values
// for getComputedStyle silently falls back to the reliable elem.style
valueIsBorderBox = isBorderBox &&
( support.boxSizingReliable() || val === elem.style[ name ] );
// Normalize "", auto, and prepare for extra
val = parseFloat( val ) || 0;
}
// Use the active box-sizing model to add/subtract irrelevant styles
return ( val +
augmentWidthOrHeight(
elem,
name,
extra || ( isBorderBox ? "border" : "content" ),
valueIsBorderBox,
styles
)
) + "px";
}

That's quite a lot of code, btw... Original issue mentioned hidden elements as well: http://bugs.jquery.com/ticket/7225

This code also works around a number of box sizing IE bugs.

@AurelioDeRosa
Copy link
Member

Hi guys. What's the state of this issue? Will it be fixed in jQuery 3?

@arthurvr
Copy link
Member

@AurelioDeRosa Well, it's on the v3.0 milestone at least. :)

@AurelioDeRosa
Copy link
Member

@arthurvr I saw the label as well but this doesn't reply to my question.

@mgol
Copy link
Member Author

mgol commented May 25, 2015

@AurelioDeRosa It's planned for 3.0 and I'm assigned. I just need to get to it. :) I'll try to do it soon, I'd like it to land before we tag the first beta.

@mgol
Copy link
Member Author

mgol commented Jul 1, 2015

PR: #2439. Feedback welcome.

@mgol
Copy link
Member Author

mgol commented Jul 6, 2015

compat version of the PR: #2454.

@timmywil
Copy link
Member

timmywil commented Jul 7, 2015

I've added a note to the blog post.

@mgol
Copy link
Member Author

mgol commented Feb 1, 2016

As reported in #2889, Chrome is deprecating offsetWidth & offsetHeight on SVG elements. We don't officially support them but it's good to know we buy more than just having fractional data by switching to getBoundingClientRect().

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

Successfully merging a pull request may close this issue.

8 participants