refactor: make element spec processing more cosistent

[dbjorge]

This PR implements hooks for customizing how DqElements are serialized.

Opening this as a draft for @WilcoFiers to pick up since I wasn't able to get it finished before heading off. The implementation is complete and it's passing integration tests (both old and new), but it still needs unit test updates (both to test the changes and also to fix up existing tests that relied on result nodes sometimes being DqElements).

After this PR, the only usage of DqElement within axe-core itself is always of form new DqElement(node, options).toJSON(), ie, serialized immediately after construction. I think this suggests that it'd be a better design to drop the class entirely and just have a serializeVirtualNode function instead that isn't unnecessarily stateful, but that's likely too breaking. We might consider marking DqElement as deprecated, though, to try to mitigate folks getting confused and thinking that they should be using it.

Closes: #3471

[dbjorge]

The original test was configureing in the top frame but not the child frame. This isn't supported and doesn't provide the protection noHtml is intended to (the thing that the test after this one is verifying). However, previously, the original test worked despite testing an unsupported configuration because there was an extra round trip through DqElement in the top frame involved that no longer exists.

I think it's preferable to break the old pattern and demand that configure must be called consistently in each frame, so I changed the test rather than forcing a top-frame-only noHtml configure to suppress html properties from child frame results, but if that feels too concerning for backcompat reasons you could address it by adding an audit.noHtml test into the process-aggregate.js code that's filling in html from node.source.

[hamax]

Should the default serializer take into account options like options.selectors? I wonder if this could have bad performance implications. Otherwise I like that we can add custom element data to results with this. One problem I see is that custom properties wouldn't be copied over in the reporter processAggregate, so if we use this we would need to implement a custom reporter (probably having to re-implement everything in processAggregate).

[WilcoFiers]

Prior to this, node.html would be null, but relatedNode.html was "Undefined". That was a bug.

[straker]

We probably need to add the setRunOptions to run virtual rule as well.

[straker]

Adding functions onto another function hides those functions from the user. They won't get code completion nor typescript support for this. Is there a reason you didn't want to have nodeSerializer be an object with a setSerializer function?

[WilcoFiers]

Why wouldn't you be able to type / get intellisense on this? I think this would do it:

  type NodeToSpec = <T extends SerialDqElement = SerialDqElement>
    (node: VirtualNode | Element) => T;
  type MergeSpecs = <T extends SerialDqElement = SerialDqElement>
    (parentSpec: SerialDqElement, childSpec: SerialDqElement) => T;

  interface Utils {
    nodeSerializer: {
      ({ isSpec, mergeSpecs }: { isSpec?: NodeToSpec, mergeSpecs?: MergeSpecs }): void,
      isSpec: NodeToSpec,
      mergeSpecs: NodeToSpec
    }

But sure, this could also just be an object with a "set" function if you want. I like this approach better personally, but it's not a big deal.

[WilcoFiers]

As discussed, I'm replacing this with a .update() method.

[WilcoFiers]

We probably need to add the setRunOptions to run virtual rule as well.

This isn't useful, as the only time DqElement will be part of runVirtualRule is if that vNode includes an actualNode. But then if you do that, this throws becauseaxe._selectorData isn't set up correctly. Adding this won't do anything, and there's no way to test it either because it throws before these options are applied.

const fixture = document.querySelector('#fixture')
fixture.innerHTML = '<i role="invalid-role"></i>'
const vNode = axe.setup(fixture.firstElementChild);
const result = axe.runVirtualRule('aria-roles', vNode);
// TypeError: Cannot read properties of undefined (reading 'role="invalid-role"')

If there's ever a use case for it, solving for it belongs in a different PR.

Done

[dbjorge]

@WilcoFiers I just have the two most recent unresolved comments remaining (it's technically my PR so I can't "request changes")

Done

[dbjorge]

LGTM (still technically my PR, so @straker will need to approve)

[straker]

Should this also include a note about needing to pass through all properties that DqElement.mergeSpec would pass through (ancestry, selector, etc.) just like the note for toSpec?

[WilcoFiers]

I don't think that's crucial, but if you have a suggestion I'll put it in 😁