Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setAttributeNode should not change order of existing attributes #116

Closed
foolip opened this issue Nov 23, 2015 · 15 comments
Closed

setAttributeNode should not change order of existing attributes #116

foolip opened this issue Nov 23, 2015 · 15 comments

Comments

@foolip
Copy link
Member

foolip commented Nov 23, 2015

https://dom.spec.whatwg.org/#concept-element-attributes-set

Per spec, this will remove oldAttr and append the new attr. This matches Gecko, but not Blink or Edge in this test:
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3758

Here is the same test using setAttribute, where the attribute order does not change:
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3759

This could be fixed by having the concept of replacing attributes, in addition to append/remove.

CC @bzbarsky @Ms2ger

@bzbarsky
Copy link

That's probably doable. It adds more complexity into what is already a fairly complex codepath, unfortunately...

@foolip
Copy link
Member Author

foolip commented Nov 23, 2015

Is the Gecko implementation pretty much step-by-step identical to the spec? If so I can see why it would complicate matters. In Blink there's first a search for the internal index of the attribute to replace, so the structure is quite different.

@bzbarsky
Copy link

The Gecko implementation is pretty much step by step identical to the spec, with the additional complication that Attr nodes are not what's stored in the element's list. They are reified as needed; the actual thing stored in the list is just the name and value. So there is some additional complication in terms of keeping the Attr nodes and actual attributes in sync.

I think we could add a replace thing on top of all that stuff, though.

@foolip
Copy link
Member Author

foolip commented Nov 23, 2015

Right, that's the same as in Blink, and I assume every browser engine ever.

I did try to change Blink to be a step-by-step copy of the spec's algorithm, and that was how I discovered this issue. I think it would work fine, but it seems faster to converge on what Blink/Edge/WebKit already do.

@annevk
Copy link
Member

annevk commented Nov 24, 2015

So what kind of mutation record should replacing give? The same as remove/append does today? Do we really want to create special code paths just for setAttributeNode (and its three aliases)?

@foolip
Copy link
Member Author

foolip commented Nov 24, 2015

Judging only by the code, I think it would do the same as when setAttribute or setAttributeNS changes an existing attribute.

That's not perfect. It would be weird that you can replace the Attr node, but for mutation observers it would look like the attribute has changed, which shows that Attr isn't actually the internal representation.

@annevk
Copy link
Member

annevk commented Nov 24, 2015

I think that is fine personally, since mutation observers do not treat attributes as proper objects. However, I'm still wondering why we'd want to introduce this additional complexity for such an unused method. I guess because three out of four browsers already do it...

@foolip
Copy link
Member Author

foolip commented Nov 24, 2015

When I filed this bug I was thinking of the difficulty of getting this fixed in three browser engines, but setAttributeNode and many other attribute-related methods need to be changed to get the case sensitivity in line with the spec, so perhaps this is a small concern in comparison.

As for complexity, it looks like it doesn't matter much either way in Blink, so if replacing is actually a complication for Gecko and if it doesn't sit too well with the spec, then I just might change my mind on this. If so we should ask WebKit and Edge folks if they see any problem.

@annevk
Copy link
Member

annevk commented Nov 24, 2015

@travisleithead? @rniwa? @hober?

@domenic
Copy link
Member

domenic commented Nov 24, 2015

(Maybe supply a nice summary of the issue for the people you just tagged in? What are the two behaviors, what are the practical web-facing differences, which browsers do what?)

FWIW I find the spec/Gecko's behavior a decent bit less convoluted, and would kind of prefer it as such. But I am sympathetic to the shortest path to interop...

@annevk
Copy link
Member

annevk commented Nov 24, 2015

The issue is whether or not setAttributeNode() and its various aliases need to replace an attribute in place (behavior of Chromium/Edge/WebKit or can remove the existing attribute and then append a new attribute (behavior of Gecko/spec). The former would require a new primitive for a method that's extremely obscure, the latter doesn't.

Aside from that there's a question that if we do go with the former, what the mutation observer behavior should be. And it sounds like it should be the same as that of changing an attribute's value.

@foolip
Copy link
Member Author

foolip commented Nov 24, 2015

You beat me to it, but this is what I was writing:

The order of attributes can be observed in the element.attributes NamedNodeMap. When calling element.setAttributeNode(attr) where there is already an attribute with the same namespaceURI and localName, one of two things could happen:

  1. The old attribute could be removed, and the new attribute appended.
  2. The old attribute could be replaced with the new attribute in-place.

The spec and Gecko do option 1. Blink, Edge and WebKit do option 2. It's unlikely that there's any compatibility risk here, it's rather a matter of getting to an interoperable state and forgetting about these APIs.

@annevk
Copy link
Member

annevk commented Jan 1, 2016

This also affects setNamedItem() and setNamedItemNS() the way the specification is written. As I pointed out in #139 (comment) we need to decide whether this complication is worth it. I guess it is given that Gecko and the specification are the only holdouts.

@smaug----, concerns?

@annevk
Copy link
Member

annevk commented Jan 1, 2016

Judging from servo/servo#9061 (comment) all browsers have a "replace" algorithm here and issue a single mutation record. Gecko just implements the replacing via remove/append, whereas everyone else does an actual replace. Given that we should just do the replacing thing. Adjusting @nox' PR will get us there.

@annevk
Copy link
Member

annevk commented Jan 1, 2016

Fixed through #139.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants