IE8: 'XPathResult' is undefined

[krusynth]

Going back to openannotation/annotator#173, there's no support for XPath related operations in any version of IE really, so we'll need a shim here. Wicked-Good-Xpath does in fact solve most of the issues here, so I recommend adding that as part of the main package, since it affects even modern IEs.

[krusynth]

Note that even with that shim, I'm still receiving a few related errors:

should parse test range 6 (Spanning multiple nodes, textNode refs.)
AssertionError: "psum dolor sit amet, consectetuer adipiscing elit.Aliquam tincidunt mauris eu risus.Lorem sed do eiusmod tempor.Humani gene" == "Level 2\n\n\n  Lorem ipsum"

should parse test range 25 (No text node at the end and offset 0) 
AssertionError: "Header Level 2Mauris lacinia ipsum nulla, id iaculis quam egestas quis. " == "Header Level 2\n\n\n  Mauris lacinia ipsum nulla, id iaculis quam egestas quis.\n\n\n"

[krusynth]

For the first one, MockSelection.resolvePath is returning different values in Chrome vs IE8. Chrome shows what appears to be the correct values for this.resolvePath(data[0]) and this.resolvePath(data[2]) :

Header Level 2 
Lorem ipsum dolor sit amet, consectetuer adipiscing elit.

but IE8 shows:

Lorem ipsum dolor sit amet, consectetuer adipiscing elit.
Humani generis

If I change the first value of data to 0, then both return the same value:

esque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Vestibulum tortor quam, feugiat vitae, u

So that initial offset seems to be problematic due to how whitespace is handled. It looks like Chrome is returning 32 total nodes for the text whereas IE8 reports just 21 - the difference is Chrome has a bunch of nodes containing just newlines. Filtering those out yields 18 nodes.

This also means that the result is expecting a series of newlines that IE8 just doesn't have.

[krusynth]

If we level the playing field by ignoring newline-only nodes, the first test passes. This ends up looking something like:

  resolvePath: (path) ->
    if typeof path is "number"
      match = /^\s*$/m
      nodes = Util.getTextNodes($(@root))
      nodes = nodes.filter (i) ->
        !match.exec(nodes[i].data)
      nodes[path]
    else if typeof path is "string"
      this.resolveXPath(@rootXPath + path)

  resolveXPath: (xpath) ->
    document.evaluate(xpath, document, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null).singleNodeValue

textInNormedRange = (range) ->
  match = /^\s*$/m
  textNodes = Util.getTextNodes($(range.commonAncestor))
  textNodes = textNodes[textNodes.index(range.start)..textNodes.index(range.end)].get()
  textNodes = textNodes.filter (node) ->
    !match.exec(node.data)
  textNodes.reduce(((acc, next) -> acc += next.nodeValue), "")

And of course, the value to test for becomes "Level 2Lorem ipsum".

[krusynth]

Test range 25 still has issues, of course (it's only matching "Header Level 2"). All of the others work fine.

So, the TLDR here is, IE8 (and several other browsers) produce rather inconsistent results when you're reading the contents of DOM nodes that have newlines in/between them - some include this extra whitespace, others don't. As a result, it's kind of hard to test predictably.

I don't know that stripping whitespace in the test helper is necessarily the best way to handle this, but at least it's consistent. We will need to alert users of the inconsistency of course.

[krusynth]

So here's a super small patch that gives appropriate expected values for IE (so far) based on how it handles whitespace in the DOM, that leaves the rest of our testing intact. This goes right after the original testData declaration in range_spec.coffee

# Overrides for IE, due to how to DOM text nodes are handled.
if /MSIE [89]/.test( window.navigator.userAgent )
  testData[6]  = [ 8,           7,   9,          11,  "Level 2Lorem ipsum",                        "Spanning multiple nodes, textNode refs." ]
  testData[25] = [ "/h2[2]",    0,"/p[4]", 0, "Header Level 2Mauris lacinia ipsum nulla, id iaculis quam egestas quis. ", "No text node at the end and offset 0"]

[csillag]

@nickstenning @tilgovi what do you think?

[krusynth]

On further reflection, I suspect this fix won't really help us. I'm not too familiar with how we're storing the data yet, but I assume that if browsers are representing this data differently, then annotations won't attach properly across browsers?

[csillag]

It depends on which version of Annotator are we talking about.
With standard upstream Annotator, if the XPath is not the same, then they won't.

With H's fork, and and further work based on that, or based on newer Annotator versions which have that work merged, we have several alternative ways to solve that, including character position-based matching, and text matching, including some level of error tolerance.

So if this fails, it's not a total tragedy - but that won't help current upstream Annotator.

[krusynth]

I suspect there's probably a way to solve this with standard upstream annotator, but I'm not familiar enough with the current situation to propose a good solution. The current problem here is that whitepsace between nodes are not reported by all browsers - as @tilgovi said elsewhere, perhaps we should just not store those.

[krusynth]

My code further up the thread is an attempt at precisely that, fwiw.

[csillag]

Are we still talking about the XPath expressions here? What do they have to do with whitespaces? (Sorry, I can't follow - probably because I did not analyze your code changes closely enough.)

[krusynth]

Yes - we are making certain assumptions about where in the text things appear, and how far along they are at. If you calculate that with whitespace vs not-whitespace, the results are necessarily different. Assuming that Annotator is working in a similar fashion for how it's storing this data, this will be effected in the same way, I think? I guess the bigger issue here is that to resolve this as I've described probably requires breaking all existing annotations (that span across nodes). 😦 But again, I'm not super-familiar with how things are being stored, so I may be misinterpreting how big of an issue this is.

[krusynth]

I believe as-is, the end result will be that annotations will still show up, just in the wrong places (in IE8 and maybe 9)?

[krusynth]

Anecdotally, this seems to be working without issue on my ad hoc testing. But I might just have not found the thing what breaks the other thing yet.

[nickstenning]

What's the action here? I don't think we need to change the xpath-range library, do we? If wgx is provided as a shim, then this library should Just Work™ without any further modifications. If the translation from HTML source -> DOM tree is inconsistent between browsers, then that's a bigger problem which probably doesn't have anything to do with XPath.

[krusynth]

I've documented any issues that affect Annotator - even if the reason is an upstream issue. I'd say this is still relevant to xpath-range since we're actually getting and storing that data and have the opportunity to perform any normalization we want on it. But either way, these tests are failing in most versions of IE, so something needs to be done (here) for this to be considered passing.

[tilgovi]

As of 3046fc5 we shouldn't have a strict dependency on XPath being available for our simple expressions.

[tilgovi]

IE8 is now passing all the tests here with rangy and wgxpath.