Permalink
Browse files

Fix #12904: Firefox defaultDisplay with body,iframe display:none. Rep…

…ort and solution by @maranomynet; test by @rwldrn. Close gh-1030.
  • Loading branch information...
1 parent 501060e commit 41ce72d69b054365a687b3f8f44bd5e163d5a544 @gibson042 committed Nov 18, 2012
Showing with 37 additions and 56 deletions.
  1. +26 −37 src/css.js
  2. +11 −19 test/unit/css.js
View
@@ -1,4 +1,4 @@
-var curCSS, iframe, iframeDoc,
+var curCSS, iframe,
ralpha = /alpha\([^)]*\)/i,
ropacity = /opacity\s*=\s*([^)]*)/,
rposition = /^(top|right|bottom|left)$/,
@@ -445,46 +445,35 @@ function getWidthOrHeight( elem, name, extra ) {
// Try to determine the default display value of an element
function css_defaultDisplay( nodeName ) {
- if ( elemdisplay[ nodeName ] ) {
- return elemdisplay[ nodeName ];
- }
-
- var elem = jQuery( "<" + nodeName + ">" ).appendTo( document.body ),
- display = elem.css("display");
- elem.remove();
-
- // If the simple way fails,
- // get element's real default display by attaching it to a temp iframe
- if ( display === "none" || display === "" ) {
- // Use the already-created iframe if possible
- iframe = document.body.appendChild(
- iframe || jQuery.extend( document.createElement("iframe"), {
- frameBorder: 0,
- width: 0,
- height: 0
- })
- );
-
- iframe.style.cssText = "display: block !important";
-
- // 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
- // document to it; WebKit & Firefox won't allow reusing the iframe document.
- if ( !iframeDoc || !iframe.createElement ) {
- iframeDoc = ( iframe.contentWindow || iframe.contentDocument ).document;
- iframeDoc.write("<!doctype html><html><body>");
- iframeDoc.close();
+ var elem,
+ doc = document,
+ display = elemdisplay[ nodeName ];
+
+ if ( !display ) {
+ elem = jQuery( doc.createElement( nodeName ) );
+ display = elem.appendTo( doc.body ).css("display");
+ elem.remove();
+
+ // If the simple way fails, read from inside an iframe
+ if ( display === "none" || !display ) {
+ // Use the already-created iframe if possible
+ iframe = ( iframe || jQuery("<iframe frameborder='0' width='0' height='0'/>").css( "cssText", "display:block !important" ) ).appendTo( doc.documentElement );
@rwaldron
rwaldron Nov 18, 2012

This line is too long.

@maranomynet
maranomynet Nov 19, 2012

I guess you can shave off the attribute value quoting - <iframe frameborder=0 width=0 height=0/>

@gibson042
gibson042 Nov 19, 2012 owner

There's actually no difference (same size jquery.min.js.gz), and stripping the quotes would deviate from the convention observed everywhere else in the complete file, so I'd recommend against that. It's just in need of line breaking.

+
+ // Always write a new HTML skeleton so Webkit and Firefox don't choke on reuse
+ doc = ( iframe[0].contentWindow || iframe[0].contentDocument ).document;
+ doc.write("<!doctype html><html><body>");
+ doc.close();
+
+ elem = jQuery( doc.createElement( nodeName ) );
+ display = elem.appendTo( doc.body ).css("display");
+ elem.remove();
@maranomynet
maranomynet Nov 19, 2012

Why remove rather than the faster detach?

@gibson042
gibson042 Nov 19, 2012 owner

Clarity. detach implies an intention to reattach later, but I'd find it acceptable if it were changed on both lines and commented to explicitly indicate the performance motivation.

+ iframe.detach();
@rwaldron
rwaldron Nov 18, 2012

Line 460-470... why? The previous approach was written to specifically avoid using jQuery API paths. The only change that needed to be made was the addition of the cssText line

@gibson042
gibson042 Nov 18, 2012 owner

Smaller file size, and no obvious reason prefer a native API over ours (performance being one consideration, but not to the exclusion of all others). Besides, previous L452 sure isn't avoiding our code paths, and previous L482 leaks elem in oldIE.

}
- elem = iframeDoc.body.appendChild( iframeDoc.createElement(nodeName) );
-
- display = curCSS( elem, "display" );
- document.body.removeChild( iframe );
+ // Store the correct default display
+ elemdisplay[ nodeName ] = display;
}
- // Store the correct default display
- elemdisplay[ nodeName ] = display;
-
return display;
}
View
@@ -574,41 +574,33 @@ test( "show() resolves correct default display for detached nodes", function(){
test("show() resolves correct default display #10227", function() {
expect(2);
- jQuery("html").append(
+ var body = jQuery("body");
+ body.append(
"<p id='ddisplay'>a<style>body{display:none}</style></p>"
);
- equal( jQuery("body").css("display"), "none", "Initial display: none" );
+ equal( body.css("display"), "none", "Initial display: none" );
- jQuery("body").show();
-
- equal( jQuery("body").css("display"), "block", "Correct display: block" );
+ body.show();
+ equal( body.css("display"), "block", "Correct display: block" );
jQuery("#ddisplay").remove();
-
- jQuery.cache = {};
+ QUnit.expectJqData( body[0], "olddisplay" );
});
test("show() resolves correct default display when iframe display:none #12904", function() {
- expect(1);
-
- var ddisplay;
+ expect(2);
- jQuery("body").append(
+ var ddisplay = jQuery(
"<p id='ddisplay'>a<style>p{display:none}iframe{display:none !important}</style></p>"
- );
+ ).appendTo("body");
- ddisplay = jQuery("#ddisplay");
+ equal( ddisplay.css("display"), "none", "Initial display: none" );
ddisplay.show();
-
- equal(
- ddisplay.css("display"), "block", "Correct display: block"
- );
+ equal( ddisplay.css("display"), "block", "Correct display: block" );
ddisplay.remove();
-
- jQuery.cache = {};
});
test("toggle()", function() {

0 comments on commit 41ce72d

Please sign in to comment.