Fix: evaluating absolute nodeset expressions with an attribute context node

[eyelidlessness]

Fixes part of enketo/enketo#58, for absolute nodesets. There is also one test confirming that the evaluator properly handles relative nodesets, but recomputation of those expressions will need to be fixed in enketo-core.

[lognaturel]

I've confirmed for myself that prior to this change, all 7 absolute node tests fail in Chrome and pass in Firefox. My brain can't seem to accept that Chrome and Safari can't evaluate absolute expressions in the context of attribute nodes but that's a personal problem.

Before looking at the implementation, I thought there might be a performance price to pay for this. Now that I see that it's adding one conditional to some existing logic, I don't think there's any real impact.

I can't think of any other risk. The tests look very complete.

[lognaturel]

I'll let this sit for a day to see if anyone else thinks of any risk and if not we can squash and merge.

@jkuester you may find the ability to bind to attributes interesting.

[alxndrsn]

It's not obvious to me why /model/instance is a special case. Is any expression starting with / absolute rather than relative? I've checked, and the following code also passes the test suite:

          const contextNode = ( 
            expr.startsWith('/')
              ? (cN?.ownerDocument ?? cN) 
              : cN
          );

(cN is only falsy in mocha tests.)

[lognaturel]

That's a good point. I briefly had this thought but it feels to me like it's worth keeping the fix very narrow to where we expect expressions to root. Expressions that start with / but not with /model/instance would not be user-authored expressions from a form definition.

My understanding is that /model/instance is an unbounded expression which defaults to the first instance (does Transformer ever generate an expression like this?) and /model/instance/ matches expressions with predicates (e.g. to explicitly select the primary instance or another instance). expr.startsWith('/model/instance) would also match nodes with names like instanceshahajustkidding so separating the two cases out helps keep the fix narrow.

[alxndrsn]

Expressions that start with / but not with /model/instance would not be user-authored expressions from a form definition.

Thanks, this is the bit I'd missed. This code seems ODK-specific, which extended-xpath.js tries to avoid. Also it's leaving a bug for future discovery. Would it be acceptable to fix the more general case, and include tests for it? e.g.:

  it('evaluates an absolute nodeset value outside /model/instance', () => {
    const attr = getAttr('item1attr');
    const value = document.evaluate('/model', attr, null, XPathResult.STRING_TYPE).stringValue;

    assert.equal(value.replace(/\s/g, ''), 'q1value2q3value4itemvalue5Buttestingitjustincase!');
  }); 

  it('evaluates root node', () => {
    const attr = getAttr('item1attr');
    const value = document.evaluate('/*', attr, null, XPathResult.STRING_TYPE).stringValue;

    assert.equal(value.replace(/\s/g, ''), 'q1value2q3value4itemvalue5Buttestingitjustincase!');
  });

[eyelidlessness]

After discussing this with @lognaturel yesterday and some time to sleep on it, I think solving this generally is probably safe/reasonable. I've made that change and included some additional tests. I also updated the Mocha setup to include all node types on the artificial Node global to fix a few unit test failures.

Looks good to me but want to give @alxndrsn the last look.

[lognaturel]

Is it worth a comment somewhere saying that this is to address a bug in Chrome/Safari and matches the behavior Firefox already has? Or do we consider the Github history sufficient?

[alxndrsn]

Is it worth a comment somewhere saying that this is to address a bug in Chrome/Safari and matches the behavior Firefox already has? Or do we consider the Github history sufficient?

I think a comment would be appreciated.

[alxndrsn]

missing a newline here; maybe worth checking editor settings?

[alxndrsn]

Added PR for checking for this in eslint: #167

[alxndrsn]

+1 for adding a comment explaining the code in extended-xpath.js, e.g.

// workaround for Chrome, see: https://bugs.chromium.org/p/chromium/issues/detail?id=XYZ

I think the code itself makes it clear what the workaround is.