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

Regression: width() & height() return 0 for all inline elements #3571

Closed
Krinkle opened this issue Mar 16, 2017 · 17 comments
Closed

Regression: width() & height() return 0 for all inline elements #3571

Krinkle opened this issue Mar 16, 2017 · 17 comments

Comments

@Krinkle
Copy link
Member

@Krinkle Krinkle commented Mar 16, 2017

Description

Select any element on the page that doesn't have an explicit width set via a stylesheet or inline attribute and call .width().

In jQuery 1.x, 3.0.x and 3.1.1 this returns the computed width.

As of jQuery 3.2.0 it returns 0. This is breaking various downstream test suites for Wikimedia and is blocking our upgrade for now.

Link to test case

http://codepen.io/Krinkle/pen/RpjgpR

@mgol
Copy link
Member

@mgol mgol commented Mar 16, 2017

Ugh, it's hard to believe we didn't have a single unit test for that...

I've created a milestone for 3.2.1, this seems serious enough that we'd want a quick patch release before 3.3.0.

@mgol mgol added this to the 3.2.1 milestone Mar 16, 2017
@mgol
Copy link
Member

@mgol mgol commented Mar 16, 2017

Caused by #3561.

@mgol
Copy link
Member

@mgol mgol commented Mar 16, 2017

Not having style specified via stylesheet or inline is not enough to trigger the issue; the element has to have a computed display: inline value.

The underlying issue is that getComputedStyle on inline elements returns auto.

@mgol
Copy link
Member

@mgol mgol commented Mar 17, 2017

From the spec getComputedStyle(elem) returns an object with resolved values for various CSS properties for elem. The definition of the resolved value is at:
https://www.w3.org/TR/cssom-1/#resolved-value
For width resolved value is defined in the following way:

If the property applies to the element or pseudo-element and the resolved value of the display property is not none, the resolved value is the used value. Otherwise the resolved value is the computed value.

The width property doesn't apply to inline elements so the computed value is returned instead of the used one (which it doesn't have). And the computed value will always be auto (again, as the property doesn't apply to inline elements).

Whether width is set inline or in the stylesheet on a particular element doesn't matter. The issue is that .width() is broken in 3.2.0 for all inline elements.

@mgol mgol changed the title Regression: width() only works when explicitly set Regression: width() & height() return 0 for all inline elements Mar 17, 2017
@mgol
Copy link
Member

@mgol mgol commented Mar 17, 2017

I'm not sure what we can do to fix this issue and still keep #3193 (i.e. not take transforms into account while computing width/height) & #1724 (support for subpixel values for width/height) fixed. This issue proves getComputedStyle is not enough but offsetWidth doesn't support fractional values while getBoundingClientRect() takes transforms into account.

Choose your poison. :/

@Krinkle
Copy link
Member Author

@Krinkle Krinkle commented Mar 17, 2017

@mgol Yeah, the problem is that the semantic purpose behind .width() is split between two very different concepts. On the one side the use case "computed style", with the rough expectation that setting the same value in CSS would essentially be a no-op (ignoring border-box for now). This doesn't apply to inline elements since setting a width does nothing.

<span>foo</span>
<span style="width: 900px;">foo</span>

These will have the same effective width, unless inline-block or block is applied to the span. So it is true that inline elements don't have a width other than 'auto'.

On the other side, there is the use case of "visual size", which semantically maps to getBoundingClientRect() and is often used to decide where to position an element, or to decide whether text will fit in an element. (The first test failure I found when attempting to upgrade to jQuery 3.2.0 was a test failure from an old plugin called jquery.autoEllipsis)

The problem is that until transforms came along, there wasn't really any way the visual size could be different from the size behind the scenes. The missing concept here is "internally drawn size".

As for transforms, I'm not convinced that it's obvious that that they shouldn't be taken into account. I can imagine plenty of use cases where users may've used width() and expect to get the effectively drawn size on the screen. Inclusion of transforms may've very well been considered an improvement or bug fix. Anyhow, considering they weren't included before jQuery 3.0, I can see why we'd want to keep it that way. But, perhaps it's an option to change that behaviour in jQuery 4.0.

@mgol
Copy link
Member

@mgol mgol commented Mar 17, 2017

@Krinkle you're right .width() has too many responsibilities; this discussion already partially happened in #3193. The problem is any real solution for that would require decoupling those responsibilities which creates a breaking change.

As for what we can do in 3.2.1, anything we do to fix this issue will break something a previous version promised to fix; at this point I think we should just revert Timmy's PR as that will break the thing that we fixed most recently so few people will be relying on that at this point.

The real solution will have to wait for 4.0.

@craigkovatch
Copy link

@craigkovatch craigkovatch commented Mar 17, 2017

As for transforms, I'm not convinced that it's obvious that that they shouldn't be taken into account.

I agree, but I wonder if a caller-specified parameter (i.e. shouldIncludeTransforms or some such) would be the easier and more flexible path?

@HolgerJeromin
Copy link

@HolgerJeromin HolgerJeromin commented Mar 17, 2017

I agree, but I wonder if a caller-specified parameter

The whole api is designed as read: .width() and write: .width(newValue)
So adding a parameter is not easy possible.
I think we really need a new api for proper support the whole mess: .width(), .css('width')

@timmywil
Copy link
Member

@timmywil timmywil commented Mar 17, 2017

@craigkovatch @HolgerJeromin Right, but even if we added another method (or signature or whatever) to support adding transforms, there's still this issue of how to support getting width with no transforms correctly.

I wonder if falling back to offsetWidth is the best option. I still lean towards ignoring transforms and erring on the side of whole numbers rather fractional values, but only when the value we get from curCSS is auto.

@craigkovatch
Copy link

@craigkovatch craigkovatch commented Mar 17, 2017

@timmywil I agree. Better to be missing a fraction of a pixel than be unexpectedly transformed by some scaling factor.

Perhaps it would be good to reach out to W3C/Webkit/Mozilla and ask for a proper API. jQuery isn't the only library that will want to have a correct answer to this question.

timmywil added a commit to timmywil/jquery that referenced this issue Mar 18, 2017
- Also added some more general dims tests for inline
  elements for posterity

Fixes jquerygh-3571
timmywil added a commit to timmywil/jquery that referenced this issue Mar 18, 2017
- Also added some more general dims tests for inline
  elements for posterity

Fixes jquerygh-3571
timmywil added a commit to timmywil/jquery that referenced this issue Mar 18, 2017
- Also added some more general dims tests for inline
  elements for posterity

Fixes jquerygh-3571
@mohithg
Copy link

@mohithg mohithg commented Mar 19, 2017

My app relies on width() and height() on inline-elements and updating to 3.2 broke my app too. When is patch going to release?

timmywil added a commit to timmywil/jquery that referenced this issue Mar 20, 2017
@timmywil timmywil closed this in 473d2ea Mar 20, 2017
@mgol
Copy link
Member

@mgol mgol commented Mar 20, 2017

@mohithg
Copy link

@mohithg mohithg commented Mar 21, 2017

@mgol Thanks.

stevenbenner added a commit to stevenbenner/jquery-powertip that referenced this issue Mar 27, 2017
Important note: jQuery version 3.2.0 had a bug where the value
returned by `.width()` and `.height()` was always zero for inline
elements. This leads to unusual tooltip positioning.

For this reason, PowerTip cannot support jQuery 3.2.0.

Ref: jquery/jquery#3571
Ref: 23cc0d0
@Krinkle
Copy link
Member Author

@Krinkle Krinkle commented Apr 1, 2017

The fix didn't work for Android 4. Could re-open, but filed #3602 instead.

@Camzilla
Copy link

@Camzilla Camzilla commented Dec 15, 2017

@mgol surprisingly ran into this problem today when replacing a table with flex. Items in my rows have "flex: 0 0 value%" and are showing up as '0' width for .height() .innerHeight() and also .outerHeight(). I got jQuery 3.2.1 in my project, is flex accounted for?

@mgol
Copy link
Member

@mgol mgol commented Dec 15, 2017

@Camzilla This seems unrelated to this issue. Could you open a new one with a test case?

rajadain added a commit to WikiWatershed/model-my-watershed that referenced this issue Feb 6, 2018
rajadain added a commit to WikiWatershed/model-my-watershed that referenced this issue Feb 6, 2018
rajadain added a commit to WikiWatershed/model-my-watershed that referenced this issue Feb 8, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jun 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

7 participants
You can’t perform that action at this time.