Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sizzle.matches passes null context, causes document to be set to global document #311

Closed
catrope opened this issue Jan 24, 2015 · 3 comments
Assignees
Labels
Milestone

Comments

@catrope
Copy link

catrope commented Jan 24, 2015

Related to #292

If you call, say, $foo.children( 'a' ) , checking which children of $foo match the 'a' selector is done by jQuery.filter. If $foo has exactly one child, jQuery.filter will call Sizzle.matchesSelector which invokes setDocument( elem.ownerDocument ) if necessary, and then passes that document to the Sizzle constructor.

However, if the number of children of $foo isn't exactly one, jQuery.filter will call Sizzle.matches instead, which just calls the Sizzle constructor directly, passing in null for the context, causing an invocation of setDocument( window.document ) regardless of what document $foo is really in.

This means that the following code causes the document context to thrash: setDocument is invoked from every .children() call, which is detrimental to performance, since setDocument contains 300+ lines of feature/bug detection code.

var doc = document.implementation.createHTMLDocument( '' ),
    $foo = $( '<p><b>Hi</b></p>', doc ),
    $bar = $( '<p><b>Hello</b><i>world</i></p>', doc ),
    $baz = $( '<p>', doc );

// Context is doc due to $( '....', doc ) above
$baz.children( 'b' ); // Sets context to window.document
$foo.children( 'b' ); // Sets context to doc
$bar.children( 'b' ); // Sets context to window.document
$foo.children( 'i' ); // Sets context to doc
$bar.children( 'i' ); // Sets context to window.document
@timmywil
Copy link
Member

Thanks for opening an issue. We actually had a discussion about this recently and meant to open one.

@timmywil timmywil added this to the 3.0.0 milestone Jan 26, 2015
@catrope
Copy link
Author

catrope commented Jan 26, 2015

Yeah, one of my coworkers started that discussion, but I was the one writing our workarounds for it and I needed a URL to use to refer to this problem in my commit messages :)

I've also noticed anecdotally that if you do something like $foo.find( 'a' ).filter( '[href]' ) where $foo is in a foreign document, two context switches occur for every call. That's probably the same underlying issue as this one, since .filter() calls the same Sizzle internals as .children()

@timmywil
Copy link
Member

Implementation discussion here. @gibson042 or I will get to it as soon as one of us is free.

wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-VisualEditor that referenced this issue Jan 28, 2015
To avoid triggering state thrashing in Sizzle
( jquery/sizzle#311 )

Also account for the fact that missing attributes are returned
as undefined by jQuery but null by .getAttribute().

Bug: T87416
Change-Id: Ib3bc7971920c084568abb6ea0bd822d763b276f2
wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-VisualEditor that referenced this issue Jan 28, 2015
Avoiding a Sizzle context switch, see jquery/sizzle#311

Bug: T87416
Change-Id: I40dd2cb17f9c3d14d0ee1aa5b542263775feedb7
wmfgerrit pushed a commit to wikimedia/VisualEditor that referenced this issue Jan 28, 2015
Copy DOM elements into the target document before processing them,
rather than after. This means the processing is done not in the
input document, but in the view document. The latter is likely to be
the main document, and so we avoid running into
jquery/sizzle#311 causing a lot of state
thrashing in Sizzle.

Bug: T87416
Change-Id: I3fbc83ebd5c07e4040b8f22a4eff9f0e1b1d7601
@timmywil timmywil self-assigned this Feb 16, 2015
timmywil added a commit to timmywil/sizzle that referenced this issue Feb 23, 2015
@mgol mgol modified the milestones: 3.0.0, 2.2.0 Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants