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

Core: Throws an error if the document element is null #439

Merged
merged 2 commits into from Apr 9, 2019

Conversation

@dangkyokhoang
Copy link
Contributor

dangkyokhoang commented Jan 22, 2019

If Sizzle/jQuery is injected to a page before the document element is constructed (window.document.documentElement === null), it fails to initialize. I would like to reference jquery/jquery#3333 to describe the issue.

setDocument() however simply returns document in such case -- even if the variable document hasn't been assigned yet (when Sizzle is initializing).

Sizzle initializes against the default document

setDocument();

setDocument() checks for doc.documentElement

if ( doc === document || doc.nodeType !== 9 || !doc.documentElement ) {

It later produces an error TypeError: "document is undefined" in assert() though the problem was actually there when setDocument() silently returned. This makes it harder for a dev-user to know what's just happened as window.document exists but window.document.documentElement doesn't. This pull request is intended to make the error verbose.

@jsf-clabot

This comment has been minimized.

Copy link

jsf-clabot commented Jan 22, 2019

CLA assistant check
All committers have signed the CLA.

@@ -580,6 +580,9 @@ setDocument = Sizzle.setDocument = function( node ) {

// Return early if doc is invalid or already selected
if ( doc === document || doc.nodeType !== 9 || !doc.documentElement ) {
if ( !document ) {
throw new Error( "Sizzle requires the document element" );

This comment has been minimized.

Copy link
@gibson042

gibson042 Jan 25, 2019

Member

I don't want to start synthesizing errors for bad environment, but I would be willing to remove some of these guards and let errors happen naturally. Please try updating the early return condition to just if ( doc === document ) and see what happens with the test suite.

This comment has been minimized.

Copy link
@dangkyokhoang

dangkyokhoang Jan 26, 2019

Author Contributor
  • if ( doc === document ) - couldn't build, got errors

    PhantomJS 2.1.1 (Linux 0.0.0) selector class FAILED
      Died on test #24 global code@test/unit/selector.js:256:11: undefined is not a constructor (evaluating 'document.createElement("fieldset")')
      assert@dist/sizzle.js:387:33
      setDocument@dist/sizzle.js:612:29
      matchesSelector@dist/sizzle.js:988:14
      test/unit/selector.js:298:36
      runTest@node_modules/qunitjs/qunit/qunit.js:843:32
      run@node_modules/qunitjs/qunit/qunit.js:828:11
      node_modules/qunitjs/qunit/qunit.js:970:14
      process@node_modules/qunitjs/qunit/qunit.js:629:24
      begin@node_modules/qunitjs/qunit/qunit.js:611:9
      node_modules/qunitjs/qunit/qunit.js:671:9
      
      Expected 27 assertions, but 24 were run
      global code@test/unit/selector.js:256:11
    ...
    
  • if ( doc === document || doc.nodeType !== 9 ) - built, tested, no errors

    Though it's pretty much like the original version, it produces a better error TypeError: "docElem is null" if document.documentElement === null, instead of TypeError: "document is undefined"


This is not related to this PR: test/index.html every test fails on Firefox (beforeEach failed on element: context is null), Chrome wouldn't load the test (jquery.js:8526 Access to XMLHttpRequest at 'Sizzle/test/data/fixtures.html' from origin 'null' has been blocked by CORS policy: Cross origin requests are only supported for protocol schemes: http, data, chrome, chrome-extension, https.) but it works on Edge - all the tests pass, is this a known issue or should I open a new issue?

@gibson042 gibson042 merged commit 7d92424 into jquery:master Apr 9, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
mgol added a commit to mgol/sizzle that referenced this pull request Aug 19, 2019
mgol added a commit to mgol/sizzle that referenced this pull request Aug 19, 2019
This is a resubmit of jquerygh-439 that was lost during the switch from JSHint + JSCS
to ESLint

Ref jquerygh-442
Ref jquerygh-439

(cherry picked from commit 7d92424)
mgol added a commit to mgol/jquery that referenced this pull request Aug 19, 2019
With new selector code doing less convoluted support tests, it was possible
to extract a lot of logic out of setDocument & also reduce size.

This commit also backports jquery/sizzle#439 that was reverted by mistake
during a switch from JSHint + JSCS to ESLint.

Ref jquery/sizzle#442
Ref jquery/sizzle#439
mgol added a commit to mgol/jquery that referenced this pull request Aug 19, 2019
With new selector code doing less convoluted support tests, it was possible
to extract a lot of logic out of setDocument & also reduce size.

This commit also backports jquery/sizzle#439 that was reverted by mistake
during a switch from JSHint + JSCS to ESLint.

Ref jquery/sizzle#442
Ref jquery/sizzle#439
mgol added a commit to mgol/jquery that referenced this pull request Aug 19, 2019
With new selector code doing less convoluted support tests, it was possible
to extract a lot of logic out of setDocument & also reduce size.

This commit also backports jquery/sizzle#439 that was reverted by mistake
during a switch from JSHint + JSCS to ESLint.

Ref jquery/sizzle#442
Ref jquery/sizzle#439
mgol added a commit to mgol/jquery that referenced this pull request Aug 20, 2019
With new selector code doing less convoluted support tests, it was possible
to extract a lot of logic out of setDocument & also reduce size.

This commit also backports jquery/sizzle#439 that was reverted by mistake
during a switch from JSHint + JSCS to ESLint.

Ref jquery/sizzle#442
Ref jquery/sizzle#439
mgol added a commit to mgol/sizzle that referenced this pull request Aug 21, 2019
This is a resubmit of jquerygh-439 that was lost during the switch from JSHint + JSCS
to ESLint

Ref jquerygh-442
Ref jquerygh-439

(cherry picked from commit 7d92424)
mgol added a commit to mgol/jquery that referenced this pull request Aug 22, 2019
With new selector code doing less convoluted support tests, it was possible
to extract a lot of logic out of setDocument & also reduce size.

This commit also backports jquery/sizzle#439 that was reverted by mistake
during a switch from JSHint + JSCS to ESLint.

Ref jquery/sizzle#442
Ref jquery/sizzle#439
mgol added a commit to mgol/jquery that referenced this pull request Aug 26, 2019
With new selector code doing less convoluted support tests, it was possible
to extract a lot of logic out of setDocument & also reduce size.

This commit also backports jquery/sizzle#439 that was reverted by mistake
during a switch from JSHint + JSCS to ESLint.

Ref jquery/sizzle#442
Ref jquery/sizzle#439
mgol added a commit to jquery/jquery that referenced this pull request Aug 26, 2019
With new selector code doing less convoluted support tests, it was possible
to extract a lot of logic out of setDocument & also reduce size.

This commit also backports jquery/sizzle#439 that was reverted by mistake
during a switch from JSHint + JSCS to ESLint.

Closes gh-4462
Ref jquery/sizzle#442
Ref jquery/sizzle#439
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.