Migrate from document.defaultView => window. Fixes #10373 #524

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
7 participants
@rwaldron
Member

rwaldron commented Sep 28, 2011

No description provided.

@paulirish

This comment has been minimized.

Show comment
Hide comment
@paulirish

paulirish Sep 28, 2011

Member

link to ticket where i brought this up: http://bugs.jquery.com/ticket/10373

Member

paulirish commented Sep 28, 2011

link to ticket where i brought this up: http://bugs.jquery.com/ticket/10373

@paulirish

This comment has been minimized.

Show comment
Hide comment
@paulirish

paulirish Sep 28, 2011

Member

You're aliasing window.getComputedStyle as a local getComputedStyle variable but that seems needless.
Instead, just check (window.getComputedStyle) in the conditionals and use it straight out within the block.

doing this you'd save another 4 (OMG FOUR!) bytes

Member

paulirish commented Sep 28, 2011

You're aliasing window.getComputedStyle as a local getComputedStyle variable but that seems needless.
Instead, just check (window.getComputedStyle) in the conditionals and use it straight out within the block.

doing this you'd save another 4 (OMG FOUR!) bytes

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Sep 28, 2011

Member
247971   (-266) jquery.js
 93914   (-103) jquery.min.js
 33197    (-16) jquery.min.js.gz         
Member

rwaldron commented Sep 28, 2011

247971   (-266) jquery.js
 93914   (-103) jquery.min.js
 33197    (-16) jquery.min.js.gz         
@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Sep 29, 2011

this breaks cross-iframe happiness cuz before it would use the element's ownerDocument.

this breaks cross-iframe happiness cuz before it would use the element's ownerDocument.

@davidmurdoch

This comment has been minimized.

Show comment
Hide comment
@davidmurdoch

davidmurdoch Sep 29, 2011

Contributor

@jdalton, how's so? Are you making jQuery document aware? (http://bugs.jquery.com/ticket/8405#comment:10)

Contributor

davidmurdoch commented Sep 29, 2011

@jdalton, how's so? Are you making jQuery document aware? (http://bugs.jquery.com/ticket/8405#comment:10)

@mikesherov

This comment has been minimized.

Show comment
Hide comment
@mikesherov

mikesherov Sep 29, 2011

Member

@jdalton, just curious... what difference does it make if we're using the getComputedStyle function of the window instead of the same function on the ownerDocument?

Member

mikesherov commented Sep 29, 2011

@jdalton, just curious... what difference does it make if we're using the getComputedStyle function of the window instead of the same function on the ownerDocument?

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Sep 29, 2011

Member

I share the same concern as @jdalton.

Member

timmywil commented Sep 29, 2011

I share the same concern as @jdalton.

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Sep 29, 2011

Member

@jdalton thanks for the heads up, ping me on IRC :)

Member

rwaldron commented Sep 29, 2011

@jdalton thanks for the heads up, ping me on IRC :)

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Sep 29, 2011

Member

@davidmurdoch @mikesherov One of the cool things about being document agnostic is that you can have jQuery in one (i)frame/primary document and script elements in other (i)frames w/o also having jQuery loaded in all (i)frames. I'm not sure how much compat jQuery has for this, but I thought it was a rad enough feature to add to my own lib. So basically before:

  // elem is from another iframe so its document, window, & defaultView are different
  elem.ownerDocument.defaultView

Using the primary window's method kills that functionality.
The use of ownerDocument and the private getWindow() function makes me think that at one time cross-frame scripting was considered a feature.

Member

jdalton commented Sep 29, 2011

@davidmurdoch @mikesherov One of the cool things about being document agnostic is that you can have jQuery in one (i)frame/primary document and script elements in other (i)frames w/o also having jQuery loaded in all (i)frames. I'm not sure how much compat jQuery has for this, but I thought it was a rad enough feature to add to my own lib. So basically before:

  // elem is from another iframe so its document, window, & defaultView are different
  elem.ownerDocument.defaultView

Using the primary window's method kills that functionality.
The use of ownerDocument and the private getWindow() function makes me think that at one time cross-frame scripting was considered a feature.

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Sep 29, 2011

Member

This test confirms that it is iframe safe: http://jsfiddle.net/rwaldron/4NWcY/

Member

rwaldron commented Sep 29, 2011

This test confirms that it is iframe safe: http://jsfiddle.net/rwaldron/4NWcY/

@gnarf

This comment has been minimized.

Show comment
Hide comment
@gnarf

gnarf Sep 29, 2011

Member

@jdalton - Little bit of research here - getComputedStyle doesn't seem to care what window its on...

Member

gnarf commented Sep 29, 2011

@jdalton - Little bit of research here - getComputedStyle doesn't seem to care what window its on...

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Sep 29, 2011

Member

It doesn't get the same result in FF 3.6; see this test.
The reason this raised red flags for me is normally foreign nodes cause errors to be thrown when inserting/attempting-to-mix-with the primary document's nodes.

Member

jdalton commented Sep 29, 2011

It doesn't get the same result in FF 3.6; see this test.
The reason this raised red flags for me is normally foreign nodes cause errors to be thrown when inserting/attempting-to-mix-with the primary document's nodes.

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Sep 29, 2011

Member

Thanks @jdalton ... I just confirmed that FF3.6 is indeed unable to get the computed style unless called from the correct context

Perhaps when FF3.6 is EOL'ed, we can re-open this

Member

rwaldron commented Sep 29, 2011

Thanks @jdalton ... I just confirmed that FF3.6 is indeed unable to get the computed style unless called from the correct context

Perhaps when FF3.6 is EOL'ed, we can re-open this

@rwaldron rwaldron closed this Sep 29, 2011

@gnarf

This comment has been minimized.

Show comment
Hide comment
@gnarf

gnarf Sep 29, 2011

Member

lame - but awesome - thanks for finding the hole @jdalton

Member

gnarf commented Sep 29, 2011

lame - but awesome - thanks for finding the hole @jdalton

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Sep 29, 2011

Member

It doesn't work in Fennec either (produces the same result as FF 3.6)

Member

jdalton commented Sep 29, 2011

It doesn't work in Fennec either (produces the same result as FF 3.6)

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