Opened 10 years ago

Closed 6 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)

testcase.zip (926 bytes) - added by Jared Jurkiewicz 10 years ago.
Testcase demonstrating the issue.
query.js.diff (2.5 KB) - added by nic 10 years ago.
[cla][patch] fixes the "nth-child" selector (xml docs on IE7)
7869.patch (1.9 KB) - added by bill 7 years ago.
updated patch to apply to [27976], but doesn't fix test failure anymore
query.html.patch (895 bytes) - added by nic 7 years ago.
patch to dojo/tests/query/query.html
combined.patch (2.8 KB) - added by bill 7 years ago.
patch to acme.js and test file, still not working for me on IE8
combined2.patch (3.0 KB) - added by nic 7 years ago.

Download all attachments as: .zip

Change History (38)

Changed 10 years ago by Jared Jurkiewicz

Attachment: testcase.zip added

Testcase demonstrating the issue.

comment:1 Changed 10 years ago by Adam Peller

Owner: changed from anonymous to alex

comment:2 Changed 10 years ago by nic

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 10 years ago by nic

Attachment: query.js.diff added

[cla][patch] fixes the "nth-child" selector (xml docs on IE7)

comment:3 Changed 10 years ago by Jared Jurkiewicz

Component: GeneralCore

comment:4 Changed 10 years ago by alex

Status: newassigned

comment:5 Changed 10 years ago by Douglas Hays

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 10 years ago by Douglas Hays

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 10 years ago by bill

Milestone: tbd1.4

1.3rc1 has been release; bumping remaining tickets to 1.4 (except for documentation/testing tickets)

comment:8 Changed 9 years ago by bill

Component: CoreQuery

comment:9 Changed 9 years ago by James Burke

Milestone: 1.41.5

comment:10 Changed 9 years ago by Adam Peller

Milestone: 1.51.6

comment:11 Changed 8 years ago by bill

Milestone: 1.6future

(sadly) punting seemingly abandoned ticket and meta tickets to future

comment:12 Changed 8 years ago by Chris Mitchell

Owner: alex deleted
Status: assignednew

comment:13 Changed 8 years ago by Chris Mitchell

Owner: set to dylan

comment:14 Changed 8 years ago by dylan

Milestone: future1.8
Owner: changed from dylan to kriszyp
Priority: highnormal

Kris is inheriting query tickets for now.

comment:15 Changed 8 years ago by bill

Owner: changed from kriszyp to Kris Zyp

comment:16 Changed 7 years ago by ben hockey

Keywords: needsreview added
Priority: highlow

comment:17 Changed 7 years ago by bill

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 Changed 7 years ago by 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.

Changed 7 years ago by bill

Attachment: 7869.patch added

updated patch to apply to [27976], but doesn't fix test failure anymore

comment:19 Changed 7 years ago by bill

Keywords: reviewed added; needsreview removed

comment:20 Changed 7 years ago by bill

Cc: nic added

Nic - see above.

comment:21 in reply to:  18 Changed 7 years ago by nic

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 in reply to:  18 Changed 7 years ago by nic

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.

Last edited 7 years ago by bill (previous) (diff)

comment:23 in reply to:  18 Changed 7 years ago by nic

@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

Changed 7 years ago by nic

Attachment: query.html.patch added

patch to dojo/tests/query/query.html

comment:24 Changed 7 years ago by bill

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 7 years ago by bill

Attachment: combined.patch added

patch to acme.js and test file, still not working for me on IE8

Changed 7 years ago by nic

Attachment: combined2.patch added

comment:25 Changed 7 years ago by nic

This patch should work, tested on IE7/8 on windows XP and IE9/win7

comment:26 Changed 7 years ago by bill

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:28 Changed 7 years ago by bill

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 in reply to:  28 Changed 7 years ago by nic

Sounds good to me, in the meantime I'll try to fix the lite engine too. Thanks!

comment:30 Changed 7 years ago by bill

In [28117]:

Fixes XML on IE in acme engine, but not lite engine, thanks for the patch nic, refs #7869 !strict.

As the comment in the parser.html test file says, when #7869 is completely fixed please move the xml_nthchild() test from the acme section to the css2 section.

comment:31 Changed 7 years ago by Colin Snover

Milestone: 1.82.0

1.8 has been tagged; moving all outstanding tickets to next major release milestone.

comment:32 Changed 6 years ago by Kitson Kelly

Resolution: wontfix
Status: newclosed

Won't fix, since move to 2.0 milestone and 2.0 won't support IE7.

Note: See TracTickets for help on using tickets.