Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fixing borked .show() on CSS hidden elements in Firefox when iframe/body/html is also hidden (Bug 12904) #1030

Closed
wants to merge 3 commits into from

4 participants

@maranomynet

bug is demonstrated here http://jsfiddle.net/qT4Tg/3/

BTW, this bug affects the master and 1.8-stable branches equally.

@gibson042
Collaborator

Thanks for contributing! Please reference a jQuery ticket with this pull request, and add a test to test/unit/css.js that fails without these changes. Other comments are inline.

@gibson042 gibson042 commented on the diff
src/css.js
@@ -456,14 +456,19 @@ function css_defaultDisplay( nodeName ) {
// If the simple way fails,
// get element's real default display by attaching it to a temp iframe
if ( display === "none" || display === "" ) {
+ var body = document.body;
@gibson042 Collaborator

All variables should be declared together. But in this case, I don't think we need the variable at all.

Right. I forgot about the linting. Sorry.

I was merely trying to yield a better minification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gibson042 gibson042 commented on the diff
src/css.js
((5 lines not shown))
// Use the already-created iframe if possible
- iframe = document.body.appendChild(
- iframe || jQuery.extend( document.createElement("iframe"), {
- frameBorder: 0,
- width: 0,
- height: 0
- })
@gibson042 Collaborator

Why did you change this block? Does your version yield a smaller jquery.min.js.gz?

I changed it because:

Appending to .cssText needs to happen on a separate line.

...and changing .cssText after the iframe has been inserted in the dom, might cause reflows (allbeit small ones)

...and invoking jQuery.extend is inherently slower than direct property assignments

...and the resulting minified code should be approximately the same size as the original jQuery.extend version (+/- 2 bytes)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gibson042 gibson042 commented on the diff
src/css.js
((6 lines not shown))
}
- // Store the correct default display
- elemdisplay[ nodeName ] = display;
+ if ( display === "none" && (curCSS( body, "display" ) === "none" || curCSS(body.parentNode, "display") === "none") )
@gibson042 Collaborator

I could maybe see not caching useless results, but supporting .show etc. in a hidden body is a bit much, especially for this cost in size and computation.

This was already discussed here: http://bugs.jquery.com/ticket/12904

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

Hmm... I don't understand...
both the branch ("bug_12904") and the title of this request reference the ticket number - as prescribed in the bug reporting guidelines.

But here's a URL: http://bugs.jquery.com/ticket/12904

@maranomynet

At the moment, building the project and writing a proper unit test for this will require more effort from me than I can justify. If someone could help me out by taking that final step, I'd be most grateful!

Otherwise, if that's a true show-stopper, feel free to ignore the reduced jsfiddle exhibit, the pull request, the whole thing.

@gibson042
Collaborator

Sorry, I totally missed the ticket reference in your title. As for the rest, thank you again (for the report, the fiddle, and the pull request). If nothing else, we will definitely fix the hidden iframe issue.

@rwaldron rwaldron commented on the diff
src/css.js
((5 lines not shown))
// Use the already-created iframe if possible
- iframe = document.body.appendChild(
- iframe || jQuery.extend( document.createElement("iframe"), {
- frameBorder: 0,
- width: 0,
- height: 0
- })
- );
+ if ( !iframe )
@rwaldron Collaborator

Curly brace belongs on this line

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

Please review the jQuery Core Style Guide that was presented when you made the pull request

@rwaldron rwaldron referenced this pull request from a commit in rwaldron/jquery
@rwaldron rwaldron Ensure that default display iframe is display:block (FF display issue…
…). Solution by @maranomynet. Fixes #12904, closes gh-1030

Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
501060e
@gibson042 gibson042 referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@gibson042 gibson042 referenced this pull request from a commit in gibson042/jquery
@gibson042 gibson042 Fix #12904: Firefox defaultDisplay with body,iframe display:none. Rep…
…ort and solution by @maranomynet; test by @rwldrn. Close gh-1030.
41ce72d
@gibson042
Collaborator

I'm with @rwldrn on this, but you can shave 22 bytes off of master and still work around body { display: none } by appending the iframe directly under documentElement instead of body: gibson042@41ce72d

@maranomynet

@gibson042 nice!
This only leaves the odd extreme edge-case of <html> element being hidden - which I think we should just ignore - at least until someone posts a new bug report :)

@gibson042
Collaborator

This ticket is still yours to fix if you want to. Just update your bug_12904 branch, sign the CLA, and you're in!

But if you do merge from my branch, I recommend using the 7751051 tip, which speeds things up a little by using the lower-level curCSS function instead of the .css method. And if you want to go even farther and trade a few bytes for the speed of native DOM methods, that'd be fine as well.

@dmethvin
Owner

I'm closing this to keep the pull queue clear but could reopen if it was freshened.

@dmethvin dmethvin closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 24 additions and 10 deletions.
  1. +24 −10 src/css.js
View
34 src/css.js
@@ -456,14 +456,19 @@ function css_defaultDisplay( nodeName ) {
// If the simple way fails,
// get element's real default display by attaching it to a temp iframe
if ( display === "none" || display === "" ) {
+ var body = document.body;
@gibson042 Collaborator

All variables should be declared together. But in this case, I don't think we need the variable at all.

Right. I forgot about the linting. Sorry.

I was merely trying to yield a better minification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
// Use the already-created iframe if possible
- iframe = document.body.appendChild(
- iframe || jQuery.extend( document.createElement("iframe"), {
- frameBorder: 0,
- width: 0,
- height: 0
- })
@gibson042 Collaborator

Why did you change this block? Does your version yield a smaller jquery.min.js.gz?

I changed it because:

Appending to .cssText needs to happen on a separate line.

...and changing .cssText after the iframe has been inserted in the dom, might cause reflows (allbeit small ones)

...and invoking jQuery.extend is inherently slower than direct property assignments

...and the resulting minified code should be approximately the same size as the original jQuery.extend version (+/- 2 bytes)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- );
+ if ( !iframe )
@rwaldron Collaborator

Curly brace belongs on this line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ {
+ iframe = document.createElement("iframe");
+ iframe.frameBorder = 0;
+ iframe.width = 0;
+ iframe.height = 0;
+ // Hidden iframes always report display as 'none' in Firefox
+ // Note: Hidden <body> or <html> might still result in a hidden iframe (handled below)
+ iframe.style.cssText += "display: block !important;";
+ }
+ body.appendChild( iframe );
// Create a cacheable copy of the iframe document on first call.
// IE and Opera will allow us to reuse the iframeDoc without re-writing the fake HTML
@@ -477,11 +482,20 @@ function css_defaultDisplay( nodeName ) {
elem = iframeDoc.body.appendChild( iframeDoc.createElement(nodeName) );
display = curCSS( elem, "display" );
- document.body.removeChild( iframe );
+ body.removeChild( iframe );
}
- // Store the correct default display
- elemdisplay[ nodeName ] = display;
+ if ( display === "none" && (curCSS( body, "display" ) === "none" || curCSS(body.parentNode, "display") === "none") )
@gibson042 Collaborator

I could maybe see not caching useless results, but supporting .show etc. in a hidden body is a bit much, especially for this cost in size and computation.

This was already discussed here: http://bugs.jquery.com/ticket/12904

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ {
+ // <body>/<html> won't hide forever,
+ // so let's go for "block" for now, and not cache the result.
+ display = "block";
+ }
+ else
+ {
+ // Store the correct default display
+ elemdisplay[ nodeName ] = display;
+ }
return display;
}
Something went wrong with that request. Please try again.