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

Bug of find element with empty attribute in v3.4 #4435

Closed
hiz8 opened this issue Jul 16, 2019 · 15 comments · Fixed by #4510 or jquery/sizzle#461
Closed

Bug of find element with empty attribute in v3.4 #4435

hiz8 opened this issue Jul 16, 2019 · 15 comments · Fixed by #4510 or jquery/sizzle#461
Assignees
Milestone

Comments

@hiz8
Copy link

@hiz8 hiz8 commented Jul 16, 2019

Description

Hi.
After updating jQuery to 3.4, I encountered strange behavior on Edge and IE11.

Finding elements with empty attributes was successful in v3.3.
But, v3.4 and later will fail on Edge and IE11.

<div id="test">
    <input name="">
</div>
var $input = $("#test").find("[name='']");

console.log($input.length); // This will return 0, only on Edge and IE11 after updating to v3.4

Link to test case

https://jsbin.com/jitatiduqi/1/edit?html,css,js,output

If this is a bug, I'm glad if a fix is ​​considered.
Thanks.

@timmywil
Copy link
Member

@timmywil timmywil commented Jul 22, 2019

We should look into the behavior change, but it's not surprising as Edge and IE often fill in undefined attributes with empty string, making them difficult to differentiate.

@timmywil timmywil added this to the 3.4.2 milestone Jul 22, 2019
@dmethvin
Copy link
Member

@dmethvin dmethvin commented Jul 22, 2019

Probably related to #3550

@mgol
Copy link
Member

@mgol mgol commented Jul 23, 2019

I added an update at #3550. @hirofumii can you check if your issue still exists with https://code.jquery.com/jquery-git.js instead of your current copy of jQuery?

@timmywil
Copy link
Member

@timmywil timmywil commented Jul 23, 2019

I checked this against jquery-git in IE11 on browserstack and it's still failing.

@timmywil
Copy link
Member

@timmywil timmywil commented Jul 23, 2019

Ok, I figured out the difference. It seems it's due to a recent optimization in Sizzle where we tried to avoid manipulating the DOM when it wasn't necessary. Turns out you get the right result if you add the ID of the context to the selector: https://jsbin.com/vasufuv/1/edit?html,js,console,output

@timmywil timmywil added Bug and removed Needs info labels Jul 23, 2019
@hiz8
Copy link
Author

@hiz8 hiz8 commented Jul 24, 2019

Thanks, @mgol.

I tested it in my environment just in case, but unfortunately the problem was not solved.
Apparently there is another cause (surely Sizzle?).

@timmywil
Copy link
Member

@timmywil timmywil commented Jul 24, 2019

@hirofumii See my comment above. It is indeed Sizzle.

@hiz8
Copy link
Author

@hiz8 hiz8 commented Jul 25, 2019

@timmywil Oh, sorry! I saw the comment and intended to add 😄, but not...

@mgol
Copy link
Member

@mgol mgol commented Aug 12, 2019

Sizzle PR: jquery/sizzle#450

EDIT: This wasn't a PR for this issue.

@mgol
Copy link
Member

@mgol mgol commented Oct 11, 2019

@timmywil Oh, that's weird that an ID would help here. Given that observation, should we just revert the optimization (both in Sizzle & jQuery master), adding a relevant unit test? After my recent PRs, all browsers except for IE & Edge don't get the temporary ID added in most cases and, as we see, IE/Edge may need it anyway.

Also, from what I see the patch at jquery/sizzle@71fe25c is not entirely correct as it skips the temporary ID for sibling selectors, pushing a selector with a leading combinator to qSA directly which crashes and goes to a slower Sizzle route.

@mgol mgol self-assigned this Oct 11, 2019
@mgol
Copy link
Member

@mgol mgol commented Oct 13, 2019

I though about all this, my conclusions:

  1. The workaround is worth to keep even for modern browsers as it avoids tokenizing in simple cases like elem.find( ".clazz1.clazz2" ) which are most likely the vast majority of what happens in the wild. Plus, the impact in IE is significant and that's a slow browser.
  2. Prefixing with an ID selector is one way to work around the issue but it doesn't help for global selectors which we can't prefix and which don't work in IE/Edge as well.
  3. One workaround I've found is adding to the document & immediately removing an input with an empty name; suddenly those selectors then work. The problem is how to write a support test for it - such a test would require adding a temporary input with an empty ID to documentElement & in my testing that fixes the issue in IE, making the test true... Perhaps we could accept that we'll only fix the issue in jQuery 4.0 where we can just apply the workaround in IE in setDocument() as we'll most likely drop EdgeHTML support in this version.

See a PR implementing that last suggestion: #4510

Let's talk it through at the next meeting.

@mgol
Copy link
Member

@mgol mgol commented Oct 14, 2019

It looks like applying the workaround once fixes it even for other frames and the workaround even survives browser refreshes. Therefore, we'll just apply it in all browsers unconditionally; hopefully that will resolve the issue.

Two PRs are available:

@mgol mgol removed the Needs review label Oct 14, 2019
mgol added a commit to mgol/jquery that referenced this issue Oct 14, 2019
qSA in IE 11/Edge often (but not always) don't find elements with an empty
name attribute selector (`[name=""]`). Assigning a temporary name attribute to
the document root (& removing it afterwards) seems to resolve the issue.

Interestingly, IE 10 & older don't seem to have the issue.

Fixes jquerygh-4435
mgol added a commit to mgol/sizzle that referenced this issue Oct 14, 2019
qSA in IE 11/Edge often (but not always) don't find elements with an empty
name attribute selector (`[name=""]`). Assigning a temporary name attribute to
the document root (& removing it afterwards) seems to resolve the issue.

Interestingly, IE 10 & older don't seem to have the issue.

Fixes jquery/jquery#4435
mgol added a commit to mgol/sizzle that referenced this issue Oct 15, 2019
qSA in IE 11/Edge often (but not always) don't find elements with an empty
name attribute selector (`[name=""]`). Assigning a temporary name attribute to
the document root (& removing it afterwards) seems to resolve the issue.

Interestingly, IE 10 & older don't seem to have the issue.

Fixes jquery/jquery#4435
mgol added a commit to mgol/sizzle that referenced this issue Oct 15, 2019
qSA in IE 11/Edge often (but not always) don't find elements with an empty
name attribute selector (`[name=""]`). Assigning a temporary name attribute to
the document root (& removing it afterwards) seems to resolve the issue.

Interestingly, IE 10 & older don't seem to have the issue.

Fixes jquery/jquery#4435
mgol added a commit to mgol/sizzle that referenced this issue Oct 15, 2019
qSA in IE 11/Edge often (but not always) don't find elements with an empty
name attribute selector (`[name=""]`). Detect that & fall back to Sizzle
traversal.

Interestingly, IE 10 & older don't seem to have the issue.

Fixes jquery/jquery#4435
mgol added a commit to mgol/sizzle that referenced this issue Oct 15, 2019
qSA in IE 11/Edge often (but not always) don't find elements with an empty
name attribute selector (`[name=""]`). Detect that & fall back to Sizzle
traversal.

Interestingly, IE 10 & older don't seem to have the issue.

Fixes jquery/jquery#4435
mgol added a commit to mgol/sizzle that referenced this issue Oct 15, 2019
qSA in IE 11/Edge often (but not always) don't find elements with an empty
name attribute selector (`[name=""]`). Detect that & fall back to Sizzle
traversal.

Interestingly, IE 10 & older don't seem to have the issue.

Fixes jquery/jquery#4435
mgol added a commit to mgol/jquery that referenced this issue Oct 15, 2019
qSA in IE 11/Edge often (but not always) don't find elements with an empty
name attribute selector (`[name=""]`). Detect that & fall back to Sizzle
traversal.

Interestingly, IE 10 & older don't seem to have the issue.

Fixes jquerygh-4435
mgol added a commit to mgol/sizzle that referenced this issue Oct 21, 2019
qSA in IE 11/Edge often (but not always) don't find elements with an empty
name attribute selector (`[name=""]`). Detect that & fall back to Sizzle
traversal.

Interestingly, IE 10 & older don't seem to have the issue.

Fixes jquery/jquery#4435
@mgol mgol reopened this Oct 21, 2019
@mgol
Copy link
Member

@mgol mgol commented Oct 21, 2019

Sizzle PR landed: jquery/sizzle#461.

Reopening as we still need to update Sizzle for this to be fixed in jQuery.

mgol added a commit to mgol/jquery that referenced this issue Oct 21, 2019
qSA in IE 11/Edge often (but not always) don't find elements with an empty
name attribute selector (`[name=""]`). Detect that & fall back to Sizzle
traversal.

Interestingly, IE 10 & older don't seem to have the issue.

Fixes jquerygh-4435
@mgol mgol reopened this Oct 21, 2019
mgol added a commit to mgol/jquery that referenced this issue Oct 22, 2019
qSA in IE 11/Edge often (but not always) don't find elements with an empty
name attribute selector (`[name=""]`). Detect that & fall back to Sizzle
traversal.

Interestingly, IE 10 & older don't seem to have the issue.

Fixes jquerygh-4435
mgol added a commit to mgol/jquery that referenced this issue Oct 22, 2019
qSA in IE 11/Edge often (but not always) don't find elements with an empty
name attribute selector (`[name=""]`). Detect that & fall back to Sizzle
traversal.

Interestingly, IE 10 & older don't seem to have the issue.

Fixes jquerygh-4435
mgol added a commit to mgol/jquery that referenced this issue Oct 22, 2019
qSA in IE 11/Edge often (but not always) don't find elements with an empty
name attribute selector (`[name=""]`). Detect that & fall back to Sizzle
traversal.

Interestingly, IE 10 & older don't seem to have the issue.

Fixes jquerygh-4435
mgol added a commit to mgol/jquery that referenced this issue Nov 18, 2019
qSA in IE 11/Edge often (but not always) don't find elements with an empty
name attribute selector (`[name=""]`). Detect that & fall back to Sizzle
traversal.

Interestingly, IE 10 & older don't seem to have the issue.

Fixes jquerygh-4435
mgol added a commit to mgol/jquery that referenced this issue Nov 18, 2019
qSA in IE 11/Edge often (but not always) don't find elements with an empty
name attribute selector (`[name=""]`). Detect that & fall back to Sizzle
traversal.

Interestingly, IE 10 & older don't seem to have the issue.

Fixes jquerygh-4435
mgol added a commit that referenced this issue Nov 18, 2019
qSA in IE 11/Edge often (but not always) don't find elements with an empty
name attribute selector (`[name=""]`). Detect that & fall back to Sizzle
traversal.

Interestingly, IE 10 & older don't seem to have the issue.

Fixes gh-4435
Closes gh-4510
@mgol
Copy link
Member

@mgol mgol commented Nov 18, 2019

The fix for jQuery 4.0 landed in 05184cc. Re-opening as we still need to update Sizzle for jQuery 3.5.0.

@mgol mgol reopened this Nov 18, 2019
mgol added a commit to mgol/jquery that referenced this issue Mar 14, 2020
mgol added a commit that referenced this issue Mar 16, 2020
@mgol
Copy link
Member

@mgol mgol commented Mar 16, 2020

Sizzle update merged in #4641. The issue is now fixed both on master & on 3.x-stable.

@mgol mgol closed this as completed Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment