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

Fixes #14545. In-body STYLE element returns nonzero dimensions #1445

Closed
wants to merge 8 commits into from

Conversation

@grisendo
Copy link

grisendo commented Nov 21, 2013

Reported by bggardner

This also occurs with script elements
See http://bugs.jquery.com/ticket/14545

src/css.js Outdated
// See http://bugs.jquery.com/ticket/14545
if (elemNodeName === "script" || elemNodeName === "style") {
return 0;
}

This comment has been minimized.

Copy link
@gibson042

gibson042 Nov 21, 2013

Member

spaces → tabs

@gibson042

This comment has been minimized.

Copy link
Member

gibson042 commented Nov 21, 2013

This needs unit tests. Also, I'd personally prefer to see any code change in the height/width getter itself (e.g., to skip swap for reserved elements) instead of getWidthOrHeight.

@dmethvin

This comment has been minimized.

Copy link
Member

dmethvin commented Dec 19, 2013

This looks good to me now. @gibson042 ?

// In HTML5, it is now valid to have style tags outside the head tag.
// See http://bugs.jquery.com/ticket/14545
var elemNodeName = elem.nodeName.toLowerCase();
if (elemNodeName === "script" || elemNodeName === "style") {

This comment has been minimized.

Copy link
@markelog

markelog Dec 19, 2013

Member

For nodeName comparisons we generally use jQuery.nodeName method, since it devised specially for this kind of cases. And it should not give big performance overhead.

Also if requires spaces, same goes for function calls in assertions you added.

@markelog

This comment has been minimized.

Copy link
Member

markelog commented Dec 19, 2013

I'm not strongly oppose to this, but why exactly we want to do this? For any hidden element it would return it width/height but not for script or style? Those elements just have default display: none declaration block, it could be undone if needed to.

Plus we didn't have these problems before, although script element is always permitted to appear in body element, plus it's a common practise, same goes for style except for permission thing.

Only thing that change is that html5 spec has eased constraints for style element, why should we do to the opposite?

@gibson042

This comment has been minimized.

Copy link
Member

gibson042 commented Dec 19, 2013

I'm inclined to agree with @markelog... if .width() and .height() are expected to ignore display: none (which they are) and style and script elements can have nonzero dimensions (which they can, at least in current browsers), then it's better to report accurately than to force them to zero—even if that means yielding some unexpected results. We even explicitly discourage the use of .width() and height() on such elements:

Although style and script tags will report a value for .width() or height() when absolutely positioned and given display:block, it is strongly discouraged to call those methods on these tags. In addition to being a bad practice, the results may also prove unreliable.

@gibson042

This comment has been minimized.

Copy link
Member

gibson042 commented Dec 19, 2013

But it turns out that we missed that block for .width(). I'd like to leave resolution as a documentation fix: jquery/api.jquery.com#403

@dmethvin

This comment has been minimized.

Copy link
Member

dmethvin commented Mar 1, 2014

You guys made your case, I'm good with leaving this as a docs issue. Sprinkling <style> and <script> elements around a document so they can be scooped up with a * selector seems sloppy anyway.

@dmethvin dmethvin closed this Mar 1, 2014
@bggardner

This comment has been minimized.

Copy link

bggardner commented Oct 9, 2014

I am the original submitter, and I'm sorry for not following up until now. While I don't disagree with leaving this as a docs issue (and an easy worked-around with CSS script, style { height: 0; width: 0; }), I'm confused as to why everyone thinks this is "sloppy" or "bad practice". I found this bug by summing the .outerHeight()s of all the children of an element, except one, then setting that one child's height to the difference of the parent's .innerHeight() and the sum. It just so happened that one child was <style scoped>, which is valid HTML5, and scoped CSS will probably become more common as browsers begin to support it. I would appreciate some feedback as to what I'm missing, as I don't believe I had explicitly set position: absolute nor display:block. I would think that element.children(":not(script):not(style)") is much sloppier. Thanks.

@dmethvin

This comment has been minimized.

Copy link
Member

dmethvin commented Oct 9, 2014

Seems that <style scoped> is being phased out so that particular issue won't be a problem in the future.

@scottgonzalez

This comment has been minimized.

Copy link
Member

scottgonzalez commented Oct 9, 2014

Just FYI: that approach to finding available space has more nuances than manually excluding script and style elements; you have to account for positioned elements and collapsing margins. So the sloppiness factor already exists, and is much worse than you expect.

@bggardner

This comment has been minimized.

Copy link

bggardner commented Oct 9, 2014

@dmethvin I wouldn't say that. Just because browsers can't figure out how to implement it, doesn't mean it's being phased out, as it is not being considered for deprecation by the W3C, AFAIK.
@scottgonzalez You're probably right, but in my case, I was working in a relatively controlled environment and was executing the code after a resizestop event. Thanks for the heads up.
@gibson042 You say "it's better to report them accurately than force them to zero". I agree, but does "accurately" mean what the browser is reporting, or what it should be...which I can only guess that somewhere in the W3C standard it says <script> and <style> elements should not be rendered?

Also, for the sake of tidying up, this is a half-duplicate of http://bugs.jquery.com/ticket/10159 and the documentation fix for .width() and .height() has a typo: height() should be .height()

@mgol

This comment has been minimized.

Copy link
Member

mgol commented Oct 9, 2014

@dmethvin I wouldn't say that. Just because browsers can't figure out how to implement it, doesn't mean it's being phased out, as it is not being considered for deprecation by the W3C, AFAIK.

At least Google currently thinks this feature is not worth the effort with Web Components coming so it might disappear from the standard one day.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.