Skip to content

fixes #12313, regression: .height() and .width() no longer fall back to CSS if offsetWidth is undefined. #909

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

Closed
wants to merge 1 commit into from

Conversation

mikesherov
Copy link
Member

This "fix" fixes a regression in SVG, which I know we say we "won't fix", but was simple enough and was a regression in 1.8 when I changed an if from val > 0 to a guard clause of val <= 0, which I thought was the opposite. However, both undefined > 0 and undefined <= 0 are false :-|

I did not include unit tests, however, because those would require us to be testing SVG and MathML, and is not something I'm interested in. I'm interested in fixing this regression and including an explanatory comment. If this means it doesn't get merged, so be it.

Running "compare_size:files" (compare_size) task
Sizes - compared to master
    258736       (+15)  dist/jquery.js                                         
     92790        (+9)  dist/jquery.min.js                                     
     33206        (-1)  dist/jquery.min.js.gz  

@dmethvin dmethvin closed this in c078b83 Aug 28, 2012
@@ -411,7 +411,10 @@ function getWidthOrHeight( elem, name, extra ) {
valueIsBorderBox = true,
isBorderBox = jQuery.support.boxSizing && jQuery.css( elem, "boxSizing" ) === "border-box";

if ( val <= 0 ) {
// some non-html elements return undefined for offsetWidth, so check for null/undefined
// svg - https://bugzilla.mozilla.org/show_bug.cgi?id=649285
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be covered by a unit test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, see the ticket.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the PR description: "I did not include unit tests, however, because those would require us to be testing SVG and MathML, and is not something I'm interested in. I'm interested in fixing this regression and including an explanatory comment. If this means it doesn't get merged, so be it."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Args sorry

mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants