jquery / jquery

jQuery JavaScript Library
https://jquery.com
MIT License
58.96k stars 20.62k forks source link

different result of width() and height() since jQuery 3.0 #3193

Closed HolgerJeromin closed 7 years ago

HolgerJeromin commented 7 years ago

Just to provide feedback:

2439 has not only the impact returning non-integer values.

I have a CSS transform:scale(2) in a root element with a div which has css width 200px. jQuery 2.x returns 200 for .width() as it uses offsetWidth jQuery 3.0 returns 400 for .width() as it uses getBoundingClientRect()

This is a breaking change which should be at least mentioned in the upgrade guide.

dmethvin commented 7 years ago

Agreed! It's a good breaking change though, wouldn't you say?

As far as documenting, perhaps we could add a sentence to the existing item here? Either that or create an entirely new "Breaking change" item for it. Rewording the heading will break existing links.

https://jquery.com/upgrade-guide/3.0/#breaking-change-width-height-css-quot-width-quot-and-css-quot-height-quot-can-return-non-integer-values

HolgerJeromin commented 7 years ago

.width() is now more correct, yes. But it was kind of buggy since the beginning of jQuery. So providing .displayWidth() instead of changing the meaning would be less painful for my application. IMO this issue is worth a new entry in the upgrade guide.

HolgerJeromin commented 7 years ago

The current implementation has one problem. Setting and Getting via css() is not symmetric anymore:

$("#Button").css("width", 200)
$("#Button").css("width")
"400px"
dmethvin commented 7 years ago

Good point, and that doesn't seem good. Not sure how to deal with it though. It would help if .css("width") returned the CSS value and .width() could return the actual width, but that would be another breaking change no doubt and perhaps worse than leaving it inconsistent.

HolgerJeromin commented 7 years ago

Applications like mine have the defect right now. Only a few people having an issue with transform. If you change css() back half of these people are getting back a working solution (without changing own code) . The other half at least are able to adapt (without asking outerwidth themselves).
css() returning not the css but a computed value (possibly manipulated by a parent) is very surprising.

dmethvin commented 7 years ago

jQuery's .css() tries really hard to return the computed value because that's usually the value people want. Browsers often don't provide a good way to get other values anyway so that's what we're stuck with. It's worth discussing with the team though to see what might be done here.

HolgerJeromin commented 7 years ago

The change is still not in the upgrade guide. IMO this should be done as fast as possible to prevent upgraders to have the same problems as we had.

dmethvin commented 7 years ago

The reason no change has been made is because we haven't yet decided whether we should change code or docs. Once we do this ticket will be closed.

HolgerJeromin commented 7 years ago

Thanks for the explanation. But IMO new Code should at least be version 3.0.1, so a warning against version 3.0 is useful in any case.

Getfree commented 7 years ago

If I may add my opinion.... The dimensions of an element and the bounding box of an element are two different concepts. I think we can agree on that.

What you are doing with this change is mixing this two concepts, so now there is no way of consistently getting the dimensions of an element. Rather, we get the dimensions or the bounding box depending on whether there are CSS transformations applied or not.

It's an acceptable breaking change if .width() and .height() now return the bounding box rather than the dimensions as long as we have a way of getting the true dimensions. Do we have such a way?

Also, .css() is supposed to give the computed css properties (hence its name), but if now .css('width') and .css('height') give the bounding box instead, that's not just a breaking change, that's messing with the user base. It's just a huge gotcha.

HolgerJeromin commented 7 years ago

Just to add another point regarding relationship of .width() and .css('width'):

Note that .width() will always return the content width, regardless of the value of the CSS box-sizing property. [...] To avoid this penalty, use .css( "width" ) rather than .width().

This is documented behavior, have not checked if this is still valid for jQuery 3.0.

dmethvin commented 7 years ago

I think those are all valid points. As far as resolving the problem, there are conflicting concerns here, I'll just mention width but it applies to height as well.

@HolgerJeromin @Getfree What would you like these APIs to return? Let's start with that and think about how existing code might break.

Getfree commented 7 years ago

The way I see it, it's essential that we have a reliable way of getting the dimensions and position of an element no matter if the element itself or an ancestor is css-transformed.

Consider this example: jQuery 2.2: https://jsfiddle.net/dxueLvfk/ jQuery 3.1: https://jsfiddle.net/dxueLvfk/1/

The blue box wants to be exactly under the red box. But since the BODY element is transformed, bounding-box coordinates are no good.

In general, from the moment you apply css-transformations, any calculation based on elements dimensions is going to give the wrong result. Even seemingly harmless transformations like 2D-translates cause problems on jQuery 2.0 already. (.offset() and .position() give bounding-box coordinates IIRC)

I'm Ok with jQuery 3 introducing breaking changes as long as there is a way of getting true element's dimensions (and not bounding-box dimensions) when they are needed. But given that this won't be needed very often, there could be an alternate way of getting these, so that .width(), .height() and .position() can provide fractional pixels (at the expense of being useless when transformations are applied).

HolgerJeromin commented 7 years ago

I'm Ok with jQuery 3 introducing breaking changes as long as there is a way of getting true element's dimensions (and not bounding-box dimensions) when they are needed.

Exactly. My application needs a way to get dimension based information for correct positioning of complex transformed elements. Minimal example: https://jsfiddle.net/3u4tug8t/2/

Non fractional value is no disadvantage if result is not complete bogus after transforming.

timmywil commented 7 years ago

Hmm, maybe we need to use offsetWidth and offsetHeight.

Are there any cases where you'd want the dimensions with transforms applied?

gibson042 commented 7 years ago

Let's start with a summary of documented surface area (using the horizontal dimension without loss of generality):

All of these should be capable of returning fractional values, but—since they are so closely tied to the CSS box model and especially since they're all also setters—ignore transforms. In fact, the non-css methods are probably the most convenient means of getting untransformed dimensions, although if we were starting from scratch we might want to condense them together and would definitely be more consistent with naming.

For this ticket, though, I have to agree with @HolgerJeromin. We should not use getBoundingClientRect values in any of the above calls.

Getfree commented 7 years ago

Also remember that .position() and .offset() make use of getBoundingClientRect as well, which means they give wrong results when transformations are applied.

Example: https://jsfiddle.net/au6uem3p/

mgol commented 7 years ago

@Getfree I wouldn't say they give "wrong" results as they do return the element displayed position.

Going back to basics, I was wondering what are the main questions being asked that jQuery (or a browser API for that matter) should answer to. I see 3 of them related to width handling:

  1. "What is the computed/resolved value of width?". The $(node).css('width') and getComputedStyle(node).width APIs are supposed to answer that question. They shouldn't take transforms into account as transforms are only influencing the final dimensions of the element on the screen, not the width computed value. I agree our current behavior is buggy here.
  2. "How can I set the new width value of the element?". The $(node).css('width', value) and node.style.width = value APIs answer to that question. It makes sense that on the jQuery side the .css() method serves both as a getter and setter as those APIs respond to each other - if you set a particular width, you'll generally get the same one from the getter. This is BTW why I agree our current behavior of the .css('width') getter that takes transforms into account is buggy.
  3. "What are the element's displayed dimensions on the screen?". This question is about how the element is displayed on the screen so it should include transforms. On the other hand, since it's not about a single property but it takes all of the things into account this API shouldn't have a setter equivalent as it's not clear what exactly it would be setting. On the jQuery side this has been the responsibility of the .width() API, although the .css('width') getter now more or less behaves in the same way (if you ignore box-sizing).

The browser APIs have been evolving in a way that should satisfy the above conditions. There is no API to get the displayed size of the element but without taking transitions into account; asking for something like that is kind of weird, most of the time people asking this question are really asking for the value of the computed width, not the displayed width minus transforms. AFAIK there is no browser API that would answer this question as well. There is innerWidth but it's treated as obsolete and Web compatibility is the only reason why it doesn't take transforms into account as well as returns decimal values only (I hope I got this paragraph right, I'd love someone working on a browser to confirm/deny it, though. @bzbarsky?)

Now, as for the last point - our problem is that the .width() method would be fine on its own as a getter of the final displayed size of the element but it also serves as a setter for us which undermines this purpose. It doesn't make it easier that we also have the .innerWidth() and .outerWidth() methods that serve both as getters and setters. The fact that all those APIs are setters as well is actually terrible - they need to know the value of box-sizing so the style write triggers the style read which means those APIs have layout thrashing built-in. I think it's bad we have APIs like that.

If we want to leave .innerWidth(), .outerWidth() and .width() as both getters and setters as they're now it seems they should be converted to fulfill the first two use cases as @gibson042 suggested. But then we don't have any jQuery API to return the dimensions of the element on the screen while we do have the API returning the position of the element on the screen. Should we have a new API for the former?

mgol commented 7 years ago

I've tried removing the width & height hooks for .css() (saving 366 bytes gzipped in the process) and I got 94 test failures but most in the dimensions module. In the css module the only failing tests were the ones checking .css('width') on a disconnected node or with negative values. This means, though, that switching to getComputedStyle for width & height is not doable before 4.0. I also think we can't switch back to offsetWidth before 4.0 as that would break the jQuery 3.0 contract that we don't cut off fractional values. Besides, I'd really like to avoid going back to fractional values.

I'm not sure if there's anything we can do before 4.0.

mgol commented 7 years ago

Going back to position and offset for a moment - we're using getBoundingClientRect() to retrieve them and this API does take transforms into account (on purpose). John Resig wrote a while ago about why this API is awesome and how it saves both code complexity & size as well as gives a speed boost. I don't see us going back to the previous implementation, it would hurt too much on those fronts and I still think that many people will want the current behavior so going back to the previous one would hurt them as well. The problem is that .offset() serves as a setter as well... Which makes for a non-symmetrical API. I don't really know what to do about it.

dmethvin commented 7 years ago

Seems like @mgol has explained the challenges here pretty well. Any API that retrieves the actual transformed dimension or position as a single number is taking several CSS properties into account and can't be used as both a getter and setter to round-trip that single number.

What can we do before 4.0? I'd consider some of this to be regressions so even if it changes existing behavior for better compatibility with 2.x it still may be in play for a 3.x.0 release.

bzbarsky commented 7 years ago

I'd love someone working on a browser to confirm/deny it

Browsers have getComputedStyle().width for returning the "used width" in CSS terms for everything except non-replaced inlines: the layout width, ignoring transforms. I think this is what you're calling "the displayed size of the element but without taking transitions into account". So an API for this already exists, again for everything except non-replaced inlines (think <span>).

There is no browser API for returning the "computed width" in CSS terms. Put another way, if you have: <div style="width: 100px"><div></div></div> and do getComputedStyle().width on the inner div, browsers will return "100px", whereas the CSS computed width there is "auto".

innerWidth is a thing on Window, so not relevant here. There's things like offsetWidth which do return non-transformed values, and might do something sane on non-replaced inlines, but as you note are integer-only.

mgol commented 7 years ago

Browsers have getComputedStyle().width for returning the "used width" in CSS terms for everything except non-replaced inlines: the layout width, ignoring transforms. I think this is what you're calling "the displayed size of the element but without taking transitions into account". So an API for this already exists, again for everything except non-replaced inlines (think ).

I actually meant the bounding box of the element i.e. "what's the size of the box that appears on the screen", so including transforms.

There is no browser API for returning the "computed width" in CSS terms. Put another way, if you have:

and do getComputedStyle().width on the inner div, browsers will return "100px", whereas the CSS computed width there is "auto".

I meant "resolved width"; I keep using the wrong name because of how getComputedStyle is named.

innerWidth is a thing on Window, so not relevant here. There's things like offsetWidth which do return non-transformed values, and might do something sane on non-replaced inlines, but as you note are integer-only.

I meant "offsetWidth" here, I keep mixing stuff, d'oh. Post corrected.

Basically, my point was that you may either ask for a resolved value for a specific CSS property, here: width or you can ask for the dimensions of the final box as it appears on the screen (i.e. the bounding box of the element). There is no API to get the bounding box minus transforms and while for width the getComputedStyle(node).width may be a good approximation, there is no similar API that we could use for offset() - you either get transforms included (via node.getBoundingClientRect().left) or you must compute the whole thing by yourself, traversing the document which is expensive.

bzbarsky commented 7 years ago

Yes, that's a correct summary of the state of browser API.

gibson042 commented 7 years ago

What can we do before 4.0? I'd consider some of this to be regressions so even if it changes existing behavior for better compatibility with 2.x it still may be in play for a 3.x.0 release.

  • Remove getBoundingClientRect() from getWidthOrHeight so .css("width") once again accurately provides CSS "width".
  • Rename and refactor the .width/.height/.inner*/.outer* surface area to clarify that they get/set CSS content/padding/border/margin box dimensions (and therefore ignore transforms). Reimplement the existing methods as thin wrappers.
  • Separately, address .offset and .position:
  • Complete and land gh-3096 or a derivative
  • Refactor to avoid dependence upon CSS width/height hooks, allowing the hooks to be removed
  • Document that .offset( setterArg ) is not reliable for elements with transformed ancestors
  • Document that .position() is not reliable for elements with transformed ancestors
timmywil commented 7 years ago

Considering the impact of some of these changes, moving to 3.2.0. We'll get smaller issues out in a 3.1.1 first.

mgol commented 7 years ago
  • Remove getBoundingClientRect() from getWidthOrHeight so .css("width") once again accurately provides CSS "width".

By reverting them to offsetWidth, I assume? I still consider it a small breaking change but maybe that's the best solution. I assume some people will depend on having access to fractional values and changing that back may break their sites so if we want to do this, we shouldn't wait too long IMO.

  • Rename and refactor the .width/.height/.inner*/.outer* surface area to clarify that they get/set CSS content/padding/border/margin box dimensions (and therefore ignore transforms). Reimplement the existing methods as thin wrappers.

I'll reiterate that I consider those setters to be bad APIs due to having layout thrashing built in. We've tried to drastically simplify .show()/.hide() for performance reasons (among other purposes but that's how it started) and we reverted the most problematic change because for many people there was no good replacement for force-showing cascade-hidden elements. In the case of the .width/.height/.inner*/.outer* APIs that's not the case. Could we ever remove those setters?

  • Separately, address .offset and .position:
    • Complete and land gh-3096 or a derivative
    • Refactor to avoid dependence upon CSS width/height hooks, allowing the hooks to be removed
    • Document that .offset( setterArg ) is not reliable for elements with transformed ancestors
    • Document that .position() is not reliable for elements with transformed ancestors

This sounds good to me.

gibson042 commented 7 years ago
  • Remove getBoundingClientRect() from getWidthOrHeight so .css("width") once again accurately provides CSS "width".

By reverting them to offsetWidth, I assume?

No, getComputedStyle is fine, just like is used for other .css calls.

I'll reiterate that I consider those setters to be bad APIs due to having layout thrashing built in. We've tried to drastically simplify .show()/.hide() for performance reasons (among other purposes but that's how it started) and we reverted the most problematic change because for many people there was no good replacement for force-showing cascade-hidden elements. In the case of the .width/.height/.inner*/.outer* APIs that's not the case. Could we ever remove those setters?

It's possible, but as I mentioned above, I believe these methods to be the most convenient means of interacting with the CSS box model—getting rid of the setter logic would leave a vacuum. And note that the layout thrashing only comes into play with property/box-sizing mismatches (e.g., $contentBox.width( val ) and $borderBox.outerWidth( val ) are equivalent to .css( "width", val )).

mgol commented 7 years ago

I mentioned in one of my comments why I think switching to gCS would be a breaking change; we currently guarantee css('width') works even on detached elements.

gibson042 commented 7 years ago

a) we should probably deprecate that b) but disconnected nodes already fall back to inline styles, and || "0px" isn't the worst of sins

dmethvin commented 7 years ago

Our heroic efforts to get the dimensions of detached or hidden elements were (in retrospect) a mistake IMO. Deprecating them ASAP seems like a good idea and warning in Migrate if we can, but I suspect it will be a while before we could remove it.

JoolsCaesar commented 7 years ago

Are you aware that this breaks draggables in jquery UI 1.11.4?

this.helper.width( this.helper.width() );

So draggables grow/shrink whenever you move them. I have yet to test 1.12.

Wykks commented 7 years ago

It's breaking jquery-ui 1.12 draggable / resizable too

JoolsCaesar commented 7 years ago

In what way? Upgrading to 1.12 fixed the problem I was facing, as the weird width=width line has been removed.

Wykks commented 7 years ago

Sorry only resizable, not draggable. Right after upgrading from jquery 2.2.4 to jquery 3.1, resizable is broken when using CSS transform:scale (With jquery ui 1.11.4 and 1.12)

scottgonzalez commented 7 years ago

Right after upgrading from jquery 2.2.4 to jquery 3.1, resizable is broken when using CSS transform:scale

jQuery UI has never supported transforms on interactions.

craigkovatch commented 7 years ago

We just upgraded to jQuery 3 and have some hidden bugs because getting/setting with .height() is now asymmetric due to a scaling transform on a root element.

Chiming in to say this is not a "positive breaking change" and agree a different method (or flag) should be added so I can opt-in to getting "bare" or "transformed" dimensions.

But whichever route is taken, please make sure .height() and .height(x) work symmetrically.

HolgerJeromin commented 7 years ago

The upgrade guide still does not mention this issue as a breaking change! 8-/

mgol commented 7 years ago

@HolgerJeromin It doesn't mention it because we consider it a bug that we'd like to fix in jQuery 3.2.0.

craigkovatch commented 7 years ago

@mgol shouldn't it still be documented? Otherwise people have to discover it for themselves via breaking bugs.

dmethvin commented 7 years ago

This ticket is the documentation that it's a bug and that we plan on fixing it.

HolgerJeromin commented 7 years ago

BTW: Some Release Notes have a section known issues for such things.

Just wanted to add that the Syntax .css( "width", "+=200" ) is probably affected, too.

sabaca commented 7 years ago

you can use something like that (for jquery-3.1.1-min.js): /* for fix 1 */var _is_IE=window.navigator.userAgent.indexOf('MSIE ')>0; .... if( /* fix 1 */ _is_IE&& /* end fix 1 */ a.getClientRects().length&&(d=a.getBoundingClientRect()[b]), ...

craigkovatch commented 7 years ago

@dmethvin that's a disappointing response. It places the onus on every developer to search through all open issues in jQuery rather than including it in e.g. a 'breaking changes/known issues' section of release notes.

dmethvin commented 7 years ago

@craigkovatch I think we all had hoped it could be fixed more quickly. Perhaps we need to fall back to assuming this won't be fixed in 3.x and will have to wait for 4.x since it's a breaking change. Would you like to submit an addition to https://github.com/jquery/jquery.com/blob/master/pages/upgrade-guide/3.0.md to document this?

craigkovatch commented 7 years ago

@dmethvin I'm concerned that breaking in 3.x and restoring in 4.x is going to make the situation even worse. Really hoping this can still end up in a 3.x release. I'm happy to document in the upgrade-guide. Would this be a candidate for inclusion in the migration plugin?

workmanw commented 7 years ago

We were burned by this :(. Our app offers a whiteboarding-like tool that has drag/drop, element resize, and scaling. This change definitely caught us off guard. There is a lot of great discussion here. The only thing I'll add is that it's very unexpected that outerWidth would be the painted size instead of the layout size. When dealing with element dimensions in this way, it seems more desirable to have layout dimensions.

I definitely see how this is kind of quagmire because you don't want to make a breaking change for 3.x. Would you guys consider adding an argument to outerWidth or event another function to optionally restore this behavior?

gibson042 commented 7 years ago

I think we know what the 3.x fix looks like, it's just a matter of someone volunteering the time to implement it (most of the core contributors, myself included, have been pulled in other directions in the latter half of this year).

vanderlee commented 7 years ago

I too have code breaking due to this undocumented backwards incompatibility. What it the current best practice for working around this issue and getting the untransformed .outerWidth?

workmanw commented 7 years ago

@vanderlee As a work around, I added the following utility functions to our application.

function _jQuerySize(elem, name) {
  if (typeof elem === 'string') {
    elem = jQuery(elem);
  }
  if (elem instanceof jQuery) {
    elem = elem[0];
  }

  let val = jQuery.style(elem, name);
  return parseFloat(val);
}

export function outerWidth(elem) {
  return _jQuerySize(elem, 'width');
}

export function outerHeight(elem) {
  return _jQuerySize(elem, 'height');
}

Then to use it:

import { outerWidth } from 'utils/jquery.js';

outerWidth('.user-item');
// or
let $userItem = $('.user-item');
outerWidth($userItem);

You could probably even monkey patch jQuery if you have 3rd party libraries that depend on this behavior ... but that felt a bit dangerous.