Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

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

Már Örlygsson Richard Gibson Rick Waldron Dave Methvin
Már Örlygsson

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

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

Richard Gibson
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.

Richard Gibson 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;
Richard Gibson 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
Richard Gibson 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
- })
Richard Gibson 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
Richard Gibson 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") )
Richard Gibson 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
Már Örlygsson

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

Már Örlygsson

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.

Richard Gibson
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.

Rick Waldron 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 )
Rick Waldron 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
Rick Waldron
Collaborator

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

Rick Waldron rwaldron referenced this pull request from a commit in rwaldron/jquery
Rick Waldron 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
Richard Gibson gibson042 referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Richard Gibson gibson042 referenced this pull request from a commit in gibson042/jquery
Richard Gibson gibson042 Fix #12904: Firefox defaultDisplay with body,iframe display:none. Rep…
…ort and solution by @maranomynet; test by @rwldrn. Close gh-1030.
41ce72d
Richard Gibson
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

Már Örlygsson

@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 :)

Richard Gibson
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.

Dave Methvin
Owner

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

Dave Methvin 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
34 src/css.js
View
@@ -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;
Richard Gibson 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
- })
Richard Gibson 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 )
Rick Waldron 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") )
Richard Gibson 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.