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

Dimensions: fall back to offsetWidth/Height for border-box in IE #4223

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@timmywil
Member

timmywil commented Nov 12, 2018

Summary

Fixes gh-4102

If box-sizing is unreliable and box-sizing is border-box, we can use offsetWidth and offsetHeight in most situations. In the situations we can't, box-sizing seems to be reliable and we can use computed CSS. Specifically, in these cases:

  • Child of hidden element
  • Disconnected element

valueIsBorderBox should be false for SVG in IE9-11.

The downside here is that we lose fractional values for border-box dimensions in IE9-11. I can live with that, but open to review.

  • I first added a commit to include the IE launcher for debugging IE issues on Windows machines, which came in handy for this.
  • Also removed a couple unused variables.

Checklist

With @gibson042's help, got it down to +9.

timmywil added some commits Nov 12, 2018

@mgol

mgol approved these changes Nov 19, 2018

Two nits but overall LGTM.

Show resolved Hide resolved src/css.js Outdated
Show resolved Hide resolved test/data/testsuite.css
@gibson042

Applying my suggestions here would take this from +31 down to +6 bytes with no behavior change, but giving up that blocks of tests makes me think that this is the wrong approach, or at least incomplete. How would you feel about the approach from #4102 (comment) ?

Show resolved Hide resolved test/unit/dimensions.js Outdated
Show resolved Hide resolved src/css.js Outdated
Show resolved Hide resolved src/css.js Outdated
src/css.js Outdated
// Also use offsetWidth/offsetHeight for when box sizing is unreliable
// Do not use offset for elements that don't have it (e.g. SVG)
if ( typeof elem[ offsetProp ] !== "undefined" &&
( val === "auto" || ( isBorderBox && !valueIsBorderBox ) ||

This comment was marked as resolved.

@gibson042

gibson042 Nov 20, 2018

Member

valueIsBorderBox !== isBorderBox is an equivalent (but smaller) comparison.

// and children of hidden elements
if ( offsetVal || !parseFloat( val ) ) {
val = offsetVal;
}

This comment has been minimized.

@gibson042

gibson042 Nov 20, 2018

Member

Big size benefit from dropping offsetVal entirely and doing this like

val = elem[ offsetProp ] || parseFloat( val ) || 0;

Plus one more from also dropping offsetProp:

val = elem[ "offset" + dimension[ 0 ].toUpperCase() + dimension.slice( 1 ) ] ||
	parseFloat( val ) || 0;

This comment has been minimized.

@timmywil

timmywil Nov 20, 2018

Member

I can do the first, but still need the second to check for existence of the property.

This comment has been minimized.

@gibson042

gibson042 Nov 20, 2018

Member

You can check for property existence by repeating elem[ "offset" + dimension[ 0 ].toUpperCase() + dimension.slice( 1 ) ], which works in our favor here.

This comment has been minimized.

@timmywil

timmywil Nov 20, 2018

Member

I don't think we should do that, even if it's smaller.

This comment has been minimized.

@timmywil

timmywil Nov 20, 2018

Member

Although it may be moot now. Now that it falls back to val, undefined is fine.

This comment has been minimized.

@timmywil

timmywil Nov 20, 2018

Member

Oh wait, that's only if we want valueIsBorderBox to be true for SVG. From my testing, it seems like it should be false. I'll wait for your reply on the jsbin.

@timmywil

This comment has been minimized.

Member

timmywil commented Nov 20, 2018

How would you feel about the approach from #4102 (comment) ?

The issue also applies to textarea, so limiting to table elements doesn't fix all issues. Also, I prefer a solution that doesn't require checking nodeName. Even though some table elements are given special consideration in the spec as offset parents, the solution still feels like it's in the same realm as browser sniffing.

We talked about || val === elem.style[ dimension ] in the meeting. I see it as an enigmatic artifact from old code. It happens to work for a lot of cases, but has no real relation to what we want. What it's doing is checking if the computed value happens to be the same as the inline style and deciding that the value can be treated as border-box because it probably doesn't matter, but we've discovered here that's a risky assumption. Patching it up to include the table case (or the case where both values are zero) isn't really solving the problem as there could be more cases to cover that we haven't thought of yet. It's just adding band-aids to an already confusing line of code that doesn't have any logical bearing on whether a value is border-box. I'd much prefer something more explicit. I know that's difficult to accomplish given IE's loose relationship with computed border-box, but this is an attempt at addressing the real behavior in IE 9-11 and documenting that behavior with comments.

timmywil added some commits Nov 20, 2018

@gibson042

This comment has been minimized.

Member

gibson042 commented Nov 20, 2018

We talked about || val === elem.style[ dimension ] in the meeting. I see it as an enigmatic artifact from old code. It happens to work for a lot of cases, but has no real relation to what we want. What it's doing is checking if the computed value happens to be the same as the inline style and deciding that the value can be treated as border-box because it probably doesn't matter, but we've discovered here that's a risky assumption.

I agree with you here, but I'm trying to gently usher IE out the door, and bringing in such a big hammer—especially one that needs workarounds on workarounds for things like SVG—runs the risk of inviting more strain on a weak point.

Patching it up to include the table case (or the case where both values are zero) isn't really solving the problem as there could be more cases to cover that we haven't thought of yet. It's just adding band-aids to an already confusing line of code that doesn't have any logical bearing on whether a value is border-box. I'd much prefer something more explicit. I know that's difficult to accomplish given IE's loose relationship with computed border-box, but this is an attempt at addressing the real behavior in IE 9-11 and documenting that behavior with comments.

OK, fair point. I'll re-review with that in mind.

timmywil added some commits Nov 20, 2018

@timmywil

This comment has been minimized.

Member

timmywil commented Nov 21, 2018

IE9-11 are happy again with no tests skipped and size impact has been reduced to +6 bytes. Plus, the use of in to check the presence of offsetProp allows us to avoid accessing offsetWidth/offsetHeight more than once. I'm inclined to call it good. I'll wait for approval from @gibson042

width: ( size + borderBoxLoss ) + "px",
height: ( size + borderBoxLoss ) + "px"
} );
}

This comment has been minimized.

@gibson042

gibson042 Nov 21, 2018

Member

It took some self-convincing, but this test change seems to be OK. However, I do like the original style better because borderBox.innerWidth(99); … assert.equal(borderBox.innerWidth(), 99) is more direct than borderBox.innerWidth(82); … assert.equal(borderBox.innerWidth() + 17, 99), especially in comparison with the corresponding contentBox assertions (whose values are always 6 less, to account for 1px border width and 2px of padding on each edge).

borderBox evaluation diff
--- before
+++ after
 size = 100
 borderBoxLoss = 17
-borderBox.css( { width: "117px", height: "117px" } )
 i = 0
 assert.equal( contentBox.innerWidth/innerHeight(), 104 )
 assert.equal( contentBox.outerWidth/outerHeight(), 106 )
-assert.equal( borderBox.innerWidth/innerHeight(), 98 )
-assert.equal( borderBox.outerWidth/outerHeight(), 100 )
+assert.equal( borderBox.innerWidth/innerHeight() + 17, 98 )
+assert.equal( borderBox.outerWidth/outerHeight() + 17, 100 )
 i = 1
 size = 101
 contentBox.innerWidth( 104 + 1 ).innerHeight( 104 + 1 )
-borderBox.innerWidth( 98 + 1 ).innerHeight( 98 + 1 )
+borderBox.innerWidth( 81 + 1 ).innerHeight( 81 + 1 )
 assert.equal( contentBox.innerWidth/innerHeight(), 105 )
 assert.equal( contentBox.outerWidth/outerHeight(), 107 )
-assert.equal( borderBox.innerWidth/innerHeight(), 99 )
-assert.equal( borderBox.outerWidth/outerHeight(), 101 )
+assert.equal( borderBox.innerWidth/innerHeight() + 17, 99 )
+assert.equal( borderBox.outerWidth/outerHeight() + 17, 101 )
 i = 2
 size = 103
 contentBox.outerWidth( 107 + 2 ).outerHeight( 107 + 2 )
-borderBox.outerWidth( 101 + 2 ).outerHeight( 101 + 2 )
+borderBox.outerWidth( 84 + 2 ).outerHeight( 84 + 2 )
 assert.equal( contentBox.innerWidth/innerHeight(), 107 )
 assert.equal( contentBox.outerWidth/outerHeight(), 109 )
-assert.equal( borderBox.innerWidth/innerHeight(), 101 )
-assert.equal( borderBox.outerWidth/outerHeight(), 103 )
+assert.equal( borderBox.innerWidth/innerHeight() + 17, 101 )
+assert.equal( borderBox.outerWidth/outerHeight() + 17, 103 )

This comment has been minimized.

@gibson042

gibson042 Nov 21, 2018

Member

Update: No, these changes are in fact wrong. And we need that initial .css(…) call style set to reveal the bug, because the difference is a constant offset after that. But it should be manual, to avoid the special width/height code:

	if ( borderBoxLoss > 0 ) {
-		borderBox.css( {
-			width: ( size + borderBoxLoss ) + "px",
-			height: ( size + borderBoxLoss ) + "px"
-		} );
+		borderBox[ 0 ].style.width = ( size + borderBoxLoss ) + "px";
+		borderBox[ 0 ].style.height = ( size + borderBoxLoss ) + "px";
	}

This comment has been minimized.

@timmywil

timmywil Nov 21, 2018

Member

Sorry what bug does it reveal exactly? The element is still being shrunk, but we're making it shrink to the the value we expect in the assertion instead of changing the assertion. I don't really see a logical difference. Either way, we're accounting for the size difference in IE9. I don't really mind your code suggestion. I'm just not seeing any advantage other than a cosmetic one.

This comment has been minimized.

@timmywil

timmywil Nov 21, 2018

Member

I went ahead and changed it, but I'd still like to understand how the previous was wrong.

This comment has been minimized.

@gibson042

gibson042 Nov 22, 2018

Member

I think it's because of the particularly strange manifestation of #3699... we want to have a dimension set before the assertions. Things might still work otherwise, but I definitely saw some passes that should have been failures when I was experimenting. And if I remember correctly from the original ticket, there were scenarios where the reported values didn't agree with the painted dimensions.

Bottom line: "wrong" is probably too strong, but it is more reliable to ensure a set before testing against expectations.

fixup! Use getClientRects() to explicitly detect hidden/disconnected
- Also update the scrollbars tests in IE9 to account for the dimension
  difference by adding more size to account for the scrollbars
@timmywil

This comment has been minimized.

Member

timmywil commented Nov 21, 2018

It's amazing how dense this code is.

@timmywil timmywil closed this in 315199c Nov 27, 2018

@timmywil timmywil deleted the timmywil:outerwidth branch Nov 27, 2018

@timmywil timmywil added this to the 3.4.0 milestone Nov 27, 2018

@mgol

This comment has been minimized.

Member

mgol commented Nov 27, 2018

@timmywil It looks like this PR broke dimensions tests in some browsers, can you have a look? http://swarm.jquery.org/job/8420

@timmywil

This comment has been minimized.

Member

timmywil commented Nov 28, 2018

I should be able to get to it today.

@mgol

This comment has been minimized.

Member

mgol commented Dec 13, 2018

The test added in this PR still fails in Android Browser 4.0-4.3: http://swarm.jquery.org/result/3143623.

@timmywil

This comment has been minimized.

Member

timmywil commented Dec 13, 2018

come on android

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