Opened 12 years ago
Closed 8 years ago
#7869 closed defect (wontfix)
[patch] [cla] Dojo.query on XML in IE 7 fails for nth-child selectors
Reported by: | Jared Jurkiewicz | Owned by: | Kris Zyp |
---|---|---|---|
Priority: | low | Milestone: | 2.0 |
Component: | Query | Version: | 1.2.0 |
Keywords: | reviewed | Cc: | alex, nic |
Blocked By: | Blocking: |
Description
Dojo.query on XML in IE 7 fails for nth-child selectors
I was trying to do a basic nth-child query across an XML dom in IE7 and discovered it fails. The same testcase works great in Firefox/Safari?.
Error in firebug lite: [ XmlDoc? ] FAILURE IN QUERY! [ Error: Object doesn't support this property or method ]
Testcase will be attached.
Attachments (6)
Change History (38)
Changed 12 years ago by
Attachment: | testcase.zip added |
---|
comment:1 Changed 12 years ago by
Owner: | changed from anonymous to alex |
---|
comment:2 Changed 12 years ago by
I have almost fixed the bug, using setAttribute/getAttribute instead of node___cachedIndex? = index in the getNodeIndex function - it seems IE doesn't allow that, when node is an xml node. But there is another problem: both FF and IE don't allow setAttribute and getAttribute on the Document Node (nodeType = 9). Thoughts?
Nicola
Changed 12 years ago by
Attachment: | query.js.diff added |
---|
[cla][patch] fixes the "nth-child" selector (xml docs on IE7)
comment:3 Changed 12 years ago by
Component: | General → Core |
---|
comment:4 Changed 12 years ago by
Status: | new → assigned |
---|
comment:5 Changed 12 years ago by
This test in query.html
doh.is(dojo.byId('_foo'), dojo.query('.foo:nth-child(2)')[0]); [ Error: assertEqual() failed: expected [object HTMLDivElement] but got undefined ]
is failing consistently for me on Safari 4 public beta (528.16)/winxp - but works using dojo 1.2.3 so this is a regression. Not sure its related to this ticket. I tested using trunk [16936].
comment:6 Changed 12 years ago by
The nth-child regression in query.html using Safari 4 was caused by [16928] and apparently is not related to this ticket.
comment:7 Changed 12 years ago by
Milestone: | tbd → 1.4 |
---|
1.3rc1 has been release; bumping remaining tickets to 1.4 (except for documentation/testing tickets)
comment:8 Changed 11 years ago by
Component: | Core → Query |
---|
comment:9 Changed 11 years ago by
Milestone: | 1.4 → 1.5 |
---|
comment:10 Changed 11 years ago by
Milestone: | 1.5 → 1.6 |
---|
comment:11 Changed 10 years ago by
Milestone: | 1.6 → future |
---|
(sadly) punting seemingly abandoned ticket and meta tickets to future
comment:12 Changed 10 years ago by
Owner: | alex deleted |
---|---|
Status: | assigned → new |
comment:13 Changed 10 years ago by
Owner: | set to dylan |
---|
comment:14 Changed 10 years ago by
Milestone: | future → 1.8 |
---|---|
Owner: | changed from dylan to kriszyp |
Priority: | high → normal |
Kris is inheriting query tickets for now.
comment:15 Changed 10 years ago by
Owner: | changed from kriszyp to Kris Zyp |
---|
comment:16 Changed 9 years ago by
Keywords: | needsreview added |
---|---|
Priority: | high → low |
comment:17 Changed 9 years ago by
Summary: | Dojo.query on XML in IE 7 fails for nth-child selectors → [patch] [cla] Dojo.query on XML in IE 7 fails for nth-child selectors |
---|
comment:18 follow-ups: 21 22 23 Changed 9 years ago by
@nic - unfortunately this patch was never applied, and got stale. There's a getTag() function now to get the case sensitive tag. And the __cachedIndex etc. were renamed in [16532] to _cidx, _clen, and then in [16870] they were renamed to _i and _l. The pnc[] change was also affected by [16532].
I redid the patch against the latest code, as best I could, but the test case still fails. Can you take a look? I'll attach the updated patch.
Changed 9 years ago by
Attachment: | 7869.patch added |
---|
updated patch to apply to [27976], but doesn't fix test failure anymore
comment:19 Changed 9 years ago by
Keywords: | reviewed added; needsreview removed |
---|
comment:21 Changed 9 years ago by
Replying to bill:
@nic - unfortunately this patch was never applied, and got stale. There's a getTag() function now to get the case sensitive tag. And the __cachedIndex etc. were renamed in [16532] to _cidx, _clen, and then in [16870] they were renamed to _i and _l. The pnc[] change was also affected by [16532].
I redid the patch against the latest code, as best I could, but the test case still fails. Can you take a look? I'll attach the updated patch.
Sure, I'll take a look. Thank you!
comment:22 Changed 9 years ago by
Replying to bill:
@nic - unfortunately this patch was never applied, and got stale. There's a getTag() function now to get the case sensitive tag. And the __cachedIndex etc. were renamed in [16532] to _cidx, _clen, and then in [16870] they were renamed to _i and _l. The pnc[] change was also affected by [16532].
I redid the patch against the latest code, as best I could, but the test case still fails. Can you take a look? I'll attach the updated patch.
@bill
I changed lines 550-554 in your patch to:
root = root.nodeType != 7 ? root : root.nextSibling; // PROCESSING_INSTRUCTION_NODE[[BR]]
and it works well; I tested on IE9 (compatibility and standard mode), this weekend I'll setup two VMs to test IE8 and IE7
I used the test case with this selector "book:nth-child(4) > isbn" (the old selector returned an empty result); also, I had to remove isDebug:true to avoid runtime errors on IE.
comment:23 Changed 9 years ago by
@bill
ok, the patch works on IE7/IE8 too.
Now there is that problem related to isDebug:true; on IE7 it triggers firebug lite and it works, but on IE8 and 9 a console.log(data) in a xhr success callback (with handleAs: "xml") causes an error
comment:24 Changed 9 years ago by
Hmm, well I tried the change you suggested, but now I get two test failures instead of 1. In the new test you added, and LOG: debug: FAILED test: doh.is(query('style')[0], query(':nth-child(2)')[0]); 0 ms. This is on IE8.
I'm attaching the combined patch. What results are you getting w/this patch?
(I tried removing the isDebug:true but that didn't change anything.
Changed 9 years ago by
Attachment: | combined.patch added |
---|
patch to acme.js and test file, still not working for me on IE8
Changed 9 years ago by
Attachment: | combined2.patch added |
---|
comment:25 Changed 9 years ago by
This patch should work, tested on IE7/8 on windows XP and IE9/win7
comment:26 follow-up: 27 Changed 9 years ago by
I tested it on IE8 and it fails for me for every selector setting. Does http://bill.dojotoolkit.org/trunk/util/doh/runner.html?testModule=tests.query really work on your IE8?
comment:27 Changed 9 years ago by
Does this work for you? http://bill.dojotoolkit.org/trunk/dojo/tests/query/query.html
comment:28 follow-up: 29 Changed 9 years ago by
Yes, http://bill.dojotoolkit.org/trunk/dojo/tests/query/query.html works on IE8, as does http://bill.dojotoolkit.org/trunk/dojo/tests/query/query.html?selector=acme, but not (for example) http://bill.dojotoolkit.org/trunk/dojo/tests/query/query.html?selector=lite.
So I guess you've fixed it in acme, but it's broken in the lite engine? I could check in your fix but leave the ticket open, and for the time being modify the test to only check acme. What do you think?
comment:29 Changed 9 years ago by
Sounds good to me, in the meantime I'll try to fix the lite engine too. Thanks!
comment:31 Changed 9 years ago by
Milestone: | 1.8 → 2.0 |
---|
1.8 has been tagged; moving all outstanding tickets to next major release milestone.
comment:32 Changed 8 years ago by
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Won't fix, since move to 2.0 milestone and 2.0 won't support IE7.
Testcase demonstrating the issue.