Skip to content

/jquery

Permalink
Browse files

Ticket #8099 Performance tweaking, credits

  • Loading branch information...
@rwaldron
rwaldron committed Apr 13, 2011
1 parent 59240d3 commit a76decc47660591bb807aecf56825019da8b39fa
Unified Split
Showing with 27 additions and 23 deletions.
  1. +15 −13 src/effects.js
  2. +2 −0 test/data/testsuite.css
  3. +10 −10 test/unit/effects.js
28 src/effects.js
View file
@@ -556,35 +556,37 @@ function defaultDisplay( nodeName ) {
var elem = jQuery( "<" + nodeName + ">" ).appendTo( "body" ),
display = elem.css( "display" );

elem.remove();
elem.remove();

if ( display === "none" || display === "" ) {

// Get element's real default display by attaching it to a temp iframe
// Conritbutions from Louis Remi and Julian Aurbourg
// based on recommendation by Louis Remi

// No iframe to use yet, so create it
if ( !iframe ) {

iframe = document.createElement( "iframe" );
iframe.width = iframe.height = 0;
iframe.frameBorder = iframe.width = iframe.height = 0;

This comment has been minimized.

Sign in to view
Copy link
@jdalton

jdalton Apr 16, 2011

Member

I know Opera may still flash an iframe on the screen for a split-second. It's why I do iframe.style.display = 'none';

This comment has been minimized.

Sign in to view
Copy link
@rwaldron

rwaldron Apr 16, 2011

Author Member

I had to avoid using iframe.style.display = 'none'; (which is what I originally had) because Firefox will returned undefined for computed styles in iframes that are display none; but yes - I agree, that would've been ideal.

This comment has been minimized.

Sign in to view
Copy link
@jdalton

jdalton Apr 16, 2011

Member

Ah yes, sorry I didn't have the full context of what this patch was addressing at the time.

This comment has been minimized.

Sign in to view
Copy link
@jdalton

jdalton Apr 16, 2011

Member

for Opera ya might end up having to set iframe.style.cssText = 'position:absolute;left:-99999px;top:0'; or smth

This comment has been minimized.

Sign in to view
Copy link
@rwaldron

rwaldron Apr 17, 2011

Author Member

thanks for that, very helpful

This comment has been minimized.

Sign in to view
Copy link
@miketaylr

miketaylr May 6, 2011

Just a heads up, there's an old bug in Opera with absolutely positioned elements that are offset greater than 32766px ( or less than -32766px ). -9999px would be safer here, or -29999px or whatever.
}

document.body.appendChild( iframe );
document.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
// document to it, Webkit & Firefox won't allow reusing the iframe document
if ( !iframeDoc || !iframe.createElement ) {
iframeDoc = ( iframe.contentWindow || iframe.contentDocument ).document;

This comment has been minimized.

Sign in to view
Copy link
@jdalton

jdalton Apr 16, 2011

Member

Isn't iframe.contentDocument the document object. So that makes the code smth like document.document ?

An easy way to grab the iframe window object is to give it an always unique name (unique for repeated new iframes too) and then reference it via frames[name]
iframeDoc.write("<!doctype><html><body></body></html>");

} else {

// Reuse previous iframe
document.body.appendChild( iframe );

iframeDoc.write( "<!doctype><html><body></body></html>" );

This comment has been minimized.

Sign in to view
Copy link
@jdalton

jdalton Apr 16, 2011

Member

If I remember correctly not calling iframeDoc.close() may cause some versions of IE's throbber to continue throbbing.

This comment has been minimized.

Sign in to view
Copy link
@rwaldron

rwaldron Apr 16, 2011

Author Member

I did pretty extensive tests in IE6, 7, 8 and didn't experience this - if you can recreate it and screen grab it, that would be super helpful - thanks!

This comment has been minimized.

Sign in to view
Copy link
@jdalton

jdalton Apr 16, 2011

Member

I made a screencast showing the problem http://www.screenr.com/yja
}

elem = iframeDoc.createElement( nodeName );

iframeDoc.body.appendChild( elem );

display = jQuery( elem ).css( "display" );
display = jQuery.css( elem, "display" );

iframe.parentNode.removeChild( iframe );
document.body.removeChild( iframe );

This comment has been minimized.

Sign in to view
Copy link
@jdalton

jdalton Apr 16, 2011

Member

Could this be called before document.body is ready? Other considerations, I know FF3.5 has problems with dynamically created iframes inserted and removed before dom-load:

// Firefox 3.5+ glitches when an iframe is inserted and removed,
// from a page containing other iframes, before dom load.
// When the page loads one of the other iframes on the page will have
// its content swapped with our iframe. Though the content is swapped,
// the iframe will persist its `src` property so we check if our
// iframe has a src property and load it if found.

Also

// IE / Opera 9.25 throw security errors when trying to write to an iframe
// after the document.domain is set.
}

// Store the correct default display
2 test/data/testsuite.css
View file
@@ -112,3 +112,5 @@ div#show-tests * { display: none; }

/* 8099 changes to default styles are read correctly */
tt { display: none; }
sup { display: none; }
dfn { display: none; }
20 test/unit/effects.js
View file
@@ -164,22 +164,22 @@ test("Persist correct display value", function() {

test("show() resolves correct default display #8099", function() {
expect(7);
var bug8099 = jQuery("<tt/>").appendTo("#main"),
div8099 = jQuery("<div/>", { className: "hidden" }).appendTo("#main");
var tt8099 = jQuery("<tt/>").appendTo("body"),
dfn8099 = jQuery("<dfn/>", { html: "foo"}).appendTo("body");

equals( bug8099.css("display"), "none", "default display override for all tt" );
equals( bug8099.show().css("display"), "inline", "Correctly resolves display:inline" );
equals( tt8099.css("display"), "none", "default display override for all tt" );
equals( tt8099.show().css("display"), "inline", "Correctly resolves display:inline" );

equals( jQuery("#foo").hide().show().css("display"), "block", "Correctly resolves display:block after hide/show" );

equals( bug8099.hide().css("display"), "none", "default display override for all tt" );
equals( bug8099.show().css("display"), "inline", "Correctly resolves display:inline" );
equals( tt8099.hide().css("display"), "none", "default display override for all tt" );
equals( tt8099.show().css("display"), "inline", "Correctly resolves display:inline" );

equals( div8099.show().css("display"), "block", "default display override for all div.hidden" );
equals( div8099.hide().css("display"), "none", "Correctly resolves display:none" );
equals( dfn8099.css("display"), "none", "default display override for all dfn" );
equals( dfn8099.show().css("display"), "inline", "Correctly resolves display:inline" );

bug8099.remove();
div8099.remove();
tt8099.remove();
dfn8099.remove();

});

2 comments on commit a76decc

@david-mark

This comment has been minimized.

Sign in to view
Copy link

replied May 6, 2011

Don't take this the wrong way, but this is all so unbelievably silly and wasteful. And enough with the "show me where it fails" nonsense. Yes, you should close the document stream after writing. You don't need to see a video of a failure to take that course.
@david-mark

This comment has been minimized.

Sign in to view
Copy link

replied May 6, 2011

About the last thing you ever want to do during load is to insert a @#$! IFRAME. It's the most expensive operation in browser scripting (a whole new window).

This is why these projects are such failures. You've got inexperienced developers trying desperately to solve every "problem" (real or perceived) for every situation and "all browsers", based almost exclusively on trial and error. There aren't any ideas here, just empirical observations and guesswork.

Give it up as the world has figured out that these monoliths are just a waste of time.
Please sign in to comment.
You can’t perform that action at this time.