Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#15631 closed defect (fixed)

[patch][cla] IE9 xhr.responseXML is insufficient for lite selector engine (the default for IE9 standards mode)

Reported by: Brandon Payton Owned by: Bryan Forbes
Priority: blocker Milestone: 1.9
Component: Query Version: 1.7.3
Keywords: Cc: Bryan Forbes, cjolif
Blocked By: Blocking:

Description

This work is covered under my personal CLA.

In IE9 standards mode, the default selector engine is 'lite'. This works correctly the page DOM and with the result of DOMParser.parseFromString() but not with the DOM implementation provided by XHR.responseXML.

XHR.responseXML does not provide a querySelectorAll implementation as the page's DOM and the result of DOMParser.parseFromString() do. Any query requiring use of querySelectorAll causes an exception in the lite engine because querySelectorAll is undefined.

I can think of a few options to address this within Dojo:

  1. Always use acme query engine for IE9. This would accomodate the variation in DOM implementation, but we lose all access to a native querySelectorAll implementation.
  2. Modify the lite engine to fall back to acme when querySelectorAll isn't available. This seems safest, but it adds an unnecessary dependency for those not using the selector engine w/ XHR XML.
  3. Modify dojo/_base/xhr to manually create XML DOM from xhr.responseText when configured to handleAs 'xml'. This appears to give us a consistent DOM implementation, access to a native querySelectorAll implementation, and access to other properties available in the core IE9 DOM implementation (e.g., element.textContent).

Any other possibilities?

After some thought, I decided to create a patch for option 3, targeting IE9 specifically. In the event of an error, the xml response is null, but the patch does nothing to verify an XML compatible MIME type before trying to parse xhr.responseText as XML. I don't know whether or not that is important, but I omitted it in favor of a simpler patch.

I have also attached an example.html page and an example.xml file that together demonstrate querySelectorAll availability in IE9 standards mode.

NOTE: Just before submitting this bug, I noticed that dojo/_base/xhr is deprecated in favor of dojo/request. I'm not sure how this applies to that. At the very least, this is a conversation starter.

Attachments (5)

example.xml (247 bytes) - added by Brandon Payton 7 years ago.
example.html dependency
xhr-ie9-responseXML.patch (991 bytes) - added by Brandon Payton 6 years ago.
a patch to manually parse xhr.responseText as XML in IE9 standards mode
dojo-request-example.html (1.9 KB) - added by Brandon Payton 6 years ago.
HTML example demonstrating the same issue with dojo/request/xhr
example.html (1.9 KB) - added by Brandon Payton 6 years ago.
HTML example demonstrating the issue with dojo/_base/xhr
xhr-html-tests.patch (974 bytes) - added by Brandon Payton 6 years ago.
Adds a new test case for dojo/_base/xhr

Download all attachments as: .zip

Change History (27)

Changed 7 years ago by Brandon Payton

Attachment: example.xml added

example.html dependency

comment:1 Changed 7 years ago by Brandon Payton

ATTENTION: My patch as-is likely introduces a nasty bug. I was eager to make a first contribution and forgot to test in older versions of IE. The patch assumes that the assignment (result = xhr.responseXML) never yields undefined. Since IE6 doesn't support the responseXML property, this is a poor assumption.

I'm up for creating a new patch, but since there are multiple ways to address this issue, I'll wait for feedback.

Last edited 7 years ago by Brandon Payton (previous) (diff)

comment:2 Changed 7 years ago by bill

Cc: Bryan Forbes added
Component: CoreQuery
Owner: set to Kris Zyp

Does the same issue occur with dojo/request?

I'm not sure if this should be classified as a dojo/query bug, or an XHR bug, or just a wontfix:

  • It doesn't seem like we should require (aka bloat) dojo/_base/xhr or even dojo/request's result to have a querySelectorAll() method.
  • It initially seems like the feature testing in the lite engine, or the dojo/query module itself, should handle this, but that would require branching not just on the browser capabilities but on the document being queried. Not sure if that's practical or not.

comment:3 Changed 7 years ago by Brandon Payton

Yes, the same issue occurs with dojo/request. I've attached an updated example (dojo-request-example.html).

It does seem like the fix should be in the query-related modules, but I didn't see a practical solution there. I'm interested to see what Kris thinks.

comment:4 Changed 7 years ago by Colin Snover

Status: newopen

comment:5 Changed 7 years ago by Colin Snover

Summary: IE9 xhr.responseXML is insufficient for lite selector engine (the default for IE9 standards mode)[patch][cla] IE9 xhr.responseXML is insufficient for lite selector engine (the default for IE9 standards mode)

comment:6 Changed 6 years ago by Brandon Payton

Coincidentally, IEBlog featured a post about this issue 11 days after this bug was posted: http://blogs.msdn.com/b/ie/archive/2012/07/19/xmlhttprequest-responsexml-in-ie10-release-preview.aspx

The responseXML in IE9 is an MSXML document. To get a native XML document in IE, you have to use DOMParser.

In IE9, dojo/query selects an engine based on the IE9 native DOM, but when handleAs=="xml", dojo/_base/xhr and dojo/request return MSXML documents that fail to deliver the native selector implementations expected by dojo/query.

comment:7 Changed 6 years ago by dylan

Milestone: tbd1.9
Priority: undecidedhigh

I think we should consider this fix for 1.9... Bryan and Kris, any thoughts on this?

Brandon, can you test your patch against 1.9 and update accordingly?

Changed 6 years ago by Brandon Payton

Attachment: xhr-ie9-responseXML.patch added

a patch to manually parse xhr.responseText as XML in IE9 standards mode

Changed 6 years ago by Brandon Payton

Attachment: dojo-request-example.html added

HTML example demonstrating the same issue with dojo/request/xhr

Changed 6 years ago by Brandon Payton

Attachment: example.html added

HTML example demonstrating the issue with dojo/_base/xhr

comment:8 Changed 6 years ago by Brandon Payton

I've fixed and updated the dojo/_base/xhr patch against trunk because it doesn't look like there is a 1.9 branch yet. If there is, let me know, and I will update accordingly.

Two comments about this fix:

  1. It seems like there should be has() test for DOMParser. Where should that go?
  2. Ideally, this fix would examine xhr.responseXML.querySelectorAll to observe its absence, but if the DOM behind xhr.responseXML is generated on demand, that would cause this fix to generate two DOM documents for the same XML instead of just one.

Doing the same fix for dojo/request/xhr appears to be more difficult due to the separation of response handlers from the request providers, so I'd like to hear from Bryan and Kris before pursuing that.

Changed 6 years ago by Brandon Payton

Attachment: xhr-html-tests.patch added

Adds a new test case for dojo/_base/xhr

comment:9 Changed 6 years ago by bill

Owner: changed from Kris Zyp to Bryan Forbes
Status: openassigned

comment:10 Changed 6 years ago by bill

Milestone: 1.91.10

Bumping this ticket since we are past the deadline for the 1.9RC. The fix can be put into 1.9.1 too, if desired.

comment:11 Changed 6 years ago by Bryan Forbes

Milestone: 1.101.9
Priority: highblocker

This should really be fixed since this is something that MS has talked about and shown a solution to. I'll have this fixed by tomorrow.

comment:12 Changed 6 years ago by bill

Cc: cjolif added
Milestone: 1.91.9.1

Let's put it in 1.9.1, since we already cut the RC for 1.9.0. Unless we want to cut another 1.9.0 RC for this ticket (and/or for other reasons).

comment:13 Changed 6 years ago by Colin Snover

Milestone: 1.9.11.9

No, let’s put it in 1.9.

comment:14 Changed 6 years ago by Brandon Payton

This is an embarrassing bug involving a huge loss of dojo/query functionality in a commonly used browser. Why not put this in 1.9 since we're so close to a fix?

comment:15 Changed 6 years ago by cjolif

Let me raise a (rhetorical) question about the urgency (i.e. can't wait a 1.x.1 which is usually release relatively shortly after a 1.x release) of something when suddenly it becomes urgent after a deadline especially when the release cycle lasts during several months and was very clear about the deadlines from the start.

Now about putting this in 1.9, the problem is that the more we put post-freeze deadline the more the risk we introduce a regression is increasing and thus the risk we have to delay the release. While you might not care about the release date, some others do care and actually 1.9 was mostly built around some contributions that have to be released early May (see 1.9 document) for both users benefit but also Dojo itself benefit (wrt to competition). I understand this is not easing your life, but until Dojo 2.0 and a separate release cycle for each sub projects I think what mostly drove a release should be valued more when determining a release cut than a bug that is here for months.

That said if everyone in copy of this bug is very confident with this fix from a regression perspective and if we do another RC because of a real regression (which might happen because of #16683 or #17061) there might be an opportunity to get it in. I will not object to it being committed if those two conditions are filled even though I really hope this will not put us in trouble.

Last edited 6 years ago by cjolif (previous) (diff)

comment:16 Changed 6 years ago by Bryan Forbes

In [31326]:

When there's a querySelectorAll in the DOM, but no qSA in responseXml, parse responseText with DOMParser. refs #15631 !strict

comment:17 Changed 6 years ago by Bryan Forbes

In [31327]:

When there's a querySelectorAll in the DOM, but no qSA in responseXml, parse responseText with DOMParser. (1.8 backport) refs #15631 !strict

comment:18 Changed 6 years ago by Bryan Forbes

Resolution: fixed
Status: assignedclosed

comment:19 Changed 6 years ago by bill

Resolution: fixed
Status: closedreopened

This test case doesn't work on my machine. xmlXhr.php gets an error:

Parse error: parse error in /ws/trunk/dojo/tests/request/xhrXml.php on line 8

which is:

<?xml version="1.0" encoding="UTF-8" ?>

I think we had this problem before and you had to adjust the file. Probably depends on the version of PHP.

comment:20 Changed 6 years ago by Colin Snover

Resolution: fixed
Status: reopenedclosed

In [31340]:

Fix dojo/request XML test for people that still have short_open_tag turned on in PHP. Fixes #15631.

comment:21 Changed 6 years ago by Colin Snover

In [31341]:

Fix dojo/request XML test for people that still have short_open_tag turned on in PHP. Fixes #15631.
Backport to 1.8

comment:22 Changed 6 years ago by bill

This change broke dojo/request on node, see #17138.

Note: See TracTickets for help on using tickets.