Dimensions: ignore transforms when retrieving width/height #3561

timmywil · 2017-03-06T23:25:09Z

Summary

I've removed our use of getBoundingClientRect in getWidthOrHeight. This is a stopgap that retrieves width/height ignoring transforms, without resorting to offsetWidth/Height, and without breaking existing tests.
@gibson042 suggested further refactoring and renaming, but I'm not sure all he has in mind. My main priority is fixing the bug with transforms. I think we can refactor further in future versions. Let me know if I missed something important.

Checklist

All authors have signed the CLA at https://contribute.jquery.com/CLA/
New tests have been added to show the fix or feature works
Grunt build and unit tests pass locally with these changes

mention-bot · 2017-03-06T23:25:10Z

@timmywil, thanks for your PR! By analyzing the history of the files in this pull request, we identified @markelog, @jeresig and @brandonaaron to be potential reviewers.

mgol · 2017-03-07T00:33:53Z

src/css.js

-	if ( elem.getClientRects().length ) {
-		val = elem.getBoundingClientRect()[ name ];
+	// Fall back to uncomputed css if necessary
+	if ( val < 0 || val == null ) {


This is dead code. curCSS will always get you a string so these conditions will always be false. curCSS will already get the inline style if the computed one is an empty string so this condition is already covered there.

Isn't that a change for another ticket? This is legacy code that I left alone. I just wonder why this has been here so long if it wasn't necessary.

It seems I forgot to simplify it when I migrated the code to use getBoundingClientRect.

That said, this is related to this PR - in my PR I should have just changed val <= 0 || val == null to val === nullas the code inside would be used in those cases, only the val <= 0 condition was never true. In your PR this becomes true dead code and since you even had to change this condition for the new logic to work, I think this should be removed here.

you even had to change this condition for the new logic to work

I did? It seems to me the logic is the same.

Before:

Try getBoundingClientRect

If negative, zero, null, or undefined, try getCSS.

If negative, null, or undefined, get inline style.

After:

Try getCSS

If negative, null, or undefined, get inline style.

Maybe I'm misunderstanding something, but it seems clear to me I haven't changed that condition.

That said, you may be right about this condition being dead code previously as well. I can check that and include it in this PR, but I still believe it is a separate ticket. At the very least, a separate commit.

Ok, did some testing in Chrome and IE9. Yup, this looks like dead code, but I do believe it was dead code before this PR as well. I'll include it here, but as a separate commit.

To sum up, since by changing getBoundingClientRect to getCSS the condition inside the if became always false while it wasn't that way before, I think the removal belongs in the same commit as the initial refactor.

Not that it's terribly important, though. :)

The condition val <= 0 was made more strict: val < 0.

That was a different condition. The diff might make it look that way. If you look closer, you'll notice the condition with val <= 0 was completely removed because it was checking the result from getBoundingClientRect, which was also removed. The val < 0 condition was already there checking the result from getCSS, which we now do first.

By changing what's initially tried from getBoundingClientRect to getCSS you eliminated the possibility of the value not being defined.

This was essentially true before. Again, this condition was already there after getCSS.

But I agree this is moot now since I've removed the condition. You approve now right?

diff?w=1 might make my point more clear.

Ahh, yeah, I totally mixed those two, you're right! They're so close to each other and somehow I didn't notice. 😄

LGTM then!

- getCSS already falls back to inline styles

- getCSS already falls back to inline styles Ref jquerygh-3561

gibson042

I guess my comments mirror @mgol's observations about dead code. As for the other refactoring, I was referencing #3193 (comment) and suggesting that we introduce methods like .contentBoxWidth/.paddingBoxWidth/.borderBoxWidth/.marginBoxWidth. It can definitely wait for later.

gibson042 · 2017-03-15T02:15:54Z

src/css.js

-		val = elem.getBoundingClientRect()[ name ];
+	// Computed unit is not pixels. Stop here and return.
+	if ( rnumnonpx.test( val ) ) {
+		return val;


Out of curiosity, when does this happen?

gibson042 · 2017-03-15T02:20:06Z

src/css.js

+	// 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 ] );


Since you deleted the "uncomputed" block above, I think the elem.style branch can be dropped.

mgol · 2017-03-17T00:04:04Z

This PR introduced a big regression: it broke .width() on inline elements. See #3571.

Dimensions: ignore transforms when retrieving width/height

1b54692

mgol requested changes Mar 7, 2017

View reviewed changes

CSS: remove dead code in getWidthOrHeight

0b25031

- getCSS already falls back to inline styles

timmywil modified the milestone: 3.2.0 Mar 13, 2017

mgol approved these changes Mar 13, 2017

View reviewed changes

timmywil added a commit to timmywil/jquery that referenced this pull request Mar 13, 2017

CSS: remove dead code in getWidthOrHeight

96533cd

- getCSS already falls back to inline styles Ref jquerygh-3561

timmywil closed this in c920ff6 Mar 13, 2017

timmywil deleted the 3193/transform-width branch March 13, 2017 17:33

gibson042 reviewed Mar 15, 2017

View reviewed changes

mgol mentioned this pull request Mar 16, 2017

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

Closed

hashchange mentioned this pull request Mar 23, 2017

.innerWidth(), .innerHeight() mishandling scroll bars in 3.2.x #3589

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Dimensions: ignore transforms when retrieving width/height #3561

Dimensions: ignore transforms when retrieving width/height #3561

Uh oh!

Conversation

timmywil commented Mar 6, 2017

Summary

Checklist

Uh oh!

mention-bot commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timmywil Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibson042 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgol commented Mar 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

timmywil Mar 13, 2017 •

edited

Loading