Update test assertions when default policy moves an attribute during …

[lukewarlow]

…Attr.value like setters

The draft spec PR now early returns in the case the element of the attribute changes rather than updating the value.

See whatwg/dom#1268

This matches https://whatpr.org/dom/1268.html#set-an-existing-attribute-value step 4 here.

[lukewarlow]

Can't comment on the actual lines but the 4 code blocks above this mean that move attribute to other element tests for those sinks aren't really doing what they say on the tin.

Because they're not moving the attribute to another element. They're making a brand new atribute and moving that instead?

[lukewarlow]

Shouldn't they be something like

      this.lastAttributeNode = findAttribute(element, attrNS, attrName);
      const newAttributeNode = document.createAttributeNS(attrNS, attrName);
      newAttributeNode.value = attrValue;
      return element.setAttributeNode(newAttributeNode);

This would no longer throw an InUseException but that's correct, because it wouldn't be inuse on the element anymore (because it was moved?)

I think we need a separate test for the specifics of the exceptions. Because we also need to test the ordering anyway.

[fred-wang]

I think the "move attribute node to another element" refers to the action of window.applyMutation which removes the attribute from setterData.lastAttributeNode.ownerElement and attaches it to otherElement.

Now it's true that when the ownerElement is null, we are not moving FROM the element to another element but just attaching it to another element.

And the original attribute node that was attached on the element at the beginning of the test is just not changed.

I think current code is aligned with existing documentation though:

runSetter: a function that runs the API to set the element's attribute to
the specified value. Before being executed, it may need to create an
attribute node. The attribute node before execution (existing or created)
is saved on runSetter.lastAttributeNode.

[fred-wang]

Mmh, probably we should keep the function as is and only use it for the default policy. Probably renaming it applyDefaultPolicyMaybeApplyingMutation or something like that.

We can have a separate policy that just returns the input, for the sake of creating trusted types.

[fred-wang]

I think the "move attribute node to another element" refers to the action of window.applyMutation which removes the attribute from setterData.lastAttributeNode.ownerElement and attaches it to otherElement.

Now it's true that when the ownerElement is null, we are not moving FROM the element to another element but just attaching it to another element.

And the original attribute node that was attached on the element at the beginning of the test is just not changed.

I think current code is aligned with existing documentation though:

runSetter: a function that runs the API to set the element's attribute to
the specified value. Before being executed, it may need to create an
attribute node. The attribute node before execution (existing or created)
is saved on runSetter.lastAttributeNode.

[fred-wang]

I see you are now adding an optional initialAttrValue. I think my original idea was to keep runSetter minimal so we can actually test situations when the element has no attributes and each test can decide to attach an attribute or not before running the setter. Arguably, finding the best generic APIs that works in all cases is a bit tricky...

If we do your proposed change, then we should update the documentation and make sure all versions of runSetter are consistent. I would also make the initialAttrValue optional so no attribute is attached by default (for these three cases the attribute would be mandatory though otherwise we don't have any attribute node). Common initialization can be factor out in a helper function.