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: fix computing outerWidth on SVGs #4096

Merged
merged 1 commit into from Jun 21, 2018

Conversation

Projects
None yet
4 participants
@jbedard
Contributor

jbedard commented Jun 11, 2018

Fixes gh-3964

Summary

Checklist

@timmywil

LGTM

src/css.js Outdated
@@ -130,11 +130,12 @@ function boxModelAdjustment( elem, dimension, box, isBorderBox, styles, computed
// Account for positive content-box scroll gutter when requested by providing computedVal
if ( !isBorderBox && computedVal >= 0 ) {
var val = elem[ "offset" + dimension[ 0 ].toUpperCase() + dimension.slice( 1 ) ];

This comment has been minimized.

@mgol

mgol Jun 11, 2018

Member

The offsetWidth/offsetHeight comment seems misplaced now, doesn't it?

Also, a comment about why the fallback is needed (below, at line 138) would be useful, it's not clear now.

@timmywil timmywil referenced this pull request Jun 11, 2018

Closed

Issue 3964 outerWidth of svg yields NaNpx #4025

3 of 4 tasks complete
src/css.js Outdated
@@ -130,11 +130,12 @@ function boxModelAdjustment( elem, dimension, box, isBorderBox, styles, computed
// Account for positive content-box scroll gutter when requested by providing computedVal
if ( !isBorderBox && computedVal >= 0 ) {
var val = elem[ "offset" + dimension[ 0 ].toUpperCase() + dimension.slice( 1 ) ];

This comment has been minimized.

@gibson042

gibson042 Jun 11, 2018

Member

I agree with @mgol on comment placement, and also observe that

  • defining a new variable here is against our general style, and
  • val is an overly generic name, and
  • it provides little benefit anyway, since delta will never be modified when offsetWidth/offsetHeight is unknown (computedVal - computedVal - delta - extra - 0.5 is guaranteed to be negative in this case, so Math.ceil will not be positive, so Math.max will be 0).

I think we should keep things simpler:

// offsetWidth/offsetHeight is a rounded sum of content, padding, scroll gutter, and border
// Scroll gutter is that value minus the other properties, rounded half-down to the nearest integer
delta += Math.max( 0, Math.ceil(
	elem[ "offset" + dimension[ 0 ].toUpperCase() + dimension.slice( 1 ) ] -
	computedVal -
	delta -
	extra -
	0.5

// If offsetWidth/offsetHeight is unknown, then we can't determine content-box scroll gutter
// Use an explicit zero to avoid NaN (gh-3964)
) ) || 0;
@jbedard

This comment has been minimized.

Contributor

jbedard commented Jun 12, 2018

Updated

@gibson042 I agree with everything you said. At one point the fix I had needed the var and I never really cleaned it up after. Thanks for pointing it out 👍

@mgol

mgol approved these changes Jun 13, 2018

@jbedard jbedard merged commit e743cbd into jquery:master Jun 21, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment