Preserve order of existing attributes for setAttributeNode (and all other aliases)

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: crimsteam, Assigned: bzbarsky)

Tracking

40 Branch
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(5 attachments)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151223140742

Steps to reproduce:

Only Firefox does not match to this new requirement:
https://github.com/whatwg/dom/issues/116

https://dom.spec.whatwg.org/#concept-element-attributes-set
https://dom.spec.whatwg.org/#concept-element-attributes-replace
(Assignee)

Comment 1

3 years ago
What mutation events (as opposed to mutation observer notifications) do the other UAs fire in this situation?  Note that attr mutation events include pointers to an attribute node, but only to one.  Which one, in this case?

IE seems to use the new one, based on the testcase at https://jsfiddle.net/wj08zxm3/1/ but I can't get Chrome and Safari to fire a mutation event at all...

In terms of implementation, if we want to do this is going to need changes to some _really_ fragile code.  See all the comments about ordering and rechecking assumptions in nsDOMAttributeMap::SetNamedItemNS.
(Assignee)

Comment 2

3 years ago
Created attachment 8705767 [details] [diff] [review]
part 1.  Remove the unused -nsDOMAttributeMap::RemoveAttribute
Attachment #8705767 - Flags: review?(bugs)
(Assignee)

Updated

3 years ago
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 3

3 years ago
Created attachment 8705768 [details] [diff] [review]
part 2.  Remove the unused mNsAware member of nsIAttribute
Attachment #8705768 - Flags: review?(bugs)
(Assignee)

Comment 4

3 years ago
Created attachment 8705769 [details] [diff] [review]
part 3.  Remove the unused aNsAware argument of nsDOMAttributeMap::GetAttribute
Attachment #8705769 - Flags: review?(bugs)
(Assignee)

Comment 5

3 years ago
Created attachment 8705770 [details] [diff] [review]
part 4.  Change nsDOMAttributeMap::SetNamedItemNS to not remove the existing attribute when there is one
Attachment #8705770 - Flags: review?(bugs)

Updated

3 years ago
Attachment #8705767 - Flags: review?(bugs) → review+

Comment 6

3 years ago
Comment on attachment 8705768 [details] [diff] [review]
part 2.  Remove the unused mNsAware member of nsIAttribute

So nsDOMAttributeMap::GetAttribute wouldn't need aNsAware param either...
but now I see that there is a separate patch for that.
Attachment #8705768 - Flags: review?(bugs) → review+

Updated

3 years ago
Attachment #8705769 - Flags: review?(bugs) → review+
(Reporter)

Comment 7

3 years ago
(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #1)
> What mutation events (as opposed to mutation observer notifications) do the
> other UAs fire in this situation?  Note that attr mutation events include
> pointers to an attribute node, but only to one.  Which one, in this case?
> 
> IE seems to use the new one, based on the testcase at
> https://jsfiddle.net/wj08zxm3/1/ but I can't get Chrome and Safari to fire a
> mutation event at all...
>

Webkit/Blink does not support DOMAttrModified event (https://developer.mozilla.org/en-US/docs/Web/Guide/Events/Mutation_events#Cross-browser_support) and maybe more (particularly when changing attributes).

I use this test for analyze other events type:
 
<pre id="log"></pre>
  <script>
  var eventTypes = [, "MODIFICATION", "ADDITION", "REMOVAL"];
  var div = document.createElement("div");
  document.body.appendChild(div);
  var attr1 = document.createAttribute("a");
  attr1.value = "one";
  attr1.expandolity = "attr1";
  var attr2 = document.createAttribute("a");
  attr2.value = "two";
  attr2.expandolity = "attr2";
  
  div.addEventListener("DOMAttrModified", function(e) {
    log.appendChild(document.createTextNode("Event: " + e.type + " " + e.attrName + " " + eventTypes[e.attrChange] + ", " + e.prevValue + ", " + e.newValue + " -- " + e.relatedNode.expandolity + "\n"));
  });
	
  div.addEventListener("DOMNodeInserted", function(e) {
    log.appendChild(document.createTextNode("Event: " + e.type + " " +  e.attrName + " " + e.attrChange + ", " + e.prevValue + ", " + e.newValue + " -- " + e.relatedNode.expandolity + "\n"));
  });
	
  div.addEventListener("DOMNodeInsertedIntoDocument", function(e) {
    log.appendChild(document.createTextNode("Event: " + e.type + " " +  e.attrName + " " + e.attrChange + ", " + e.prevValue + ", " + e.newValue + " -- " + e.relatedNode.expandolity + "\n"));
  });
	
  div.addEventListener("DOMNodeRemoved", function(e) {
    log.appendChild(document.createTextNode("Event: " + e.type + " " +  e.attrName + " " + e.attrChange + ", " + e.prevValue + ", " + e.newValue + " -- " + e.relatedNode.expandolity + "\n"));
  });	
	
  div.addEventListener("DOMNodeRemovedFromDocument", function(e) {
    log.appendChild(document.createTextNode("Event: " + e.type + " " +  e.attrName + " " + e.attrChange + ", " + e.prevValue + ", " + e.newValue + " -- " + e.relatedNode.expandolity + "\n"));
  });	
	
  div.addEventListener("DOMSubtreeModified", function(e) {
    log.appendChild(document.createTextNode("Event: " + e.type + " " +  e.attrName + " " + e.attrChange + ", " + e.prevValue + ", " + e.newValue + " -- " + e.relatedNode + "\n"));
  });
	
  div.setAttributeNode(attr1);
  div.setAttributeNode(attr2);

</script>

Firefox 43:
Event: DOMAttrModified a ADDITION, , one -- attr1
Event: DOMSubtreeModified  0, ,  -- null
Event: DOMAttrModified a MODIFICATION, one, two -- attr2
Event: DOMSubtreeModified  0, ,  -- null

Firefox 46.01 (here something had to change when comparing to Firefox 43 behavior)
Event: DOMAttrModified a ADDITION, , one -- attr1
Event: DOMSubtreeModified  0, ,  -- null
Event: DOMAttrModified a REMOVAL, one,  -- attr1
Event: DOMSubtreeModified  0, ,  -- null
Event: DOMAttrModified a ADDITION, , two -- attr2
Event: DOMSubtreeModified  0, ,  -- null

IE 11 (very similar as Firefox 43):
Event: DOMAttrModified a ADDITION, , one -- attr1
Event: DOMAttrModified a MODIFICATION, one, two -- attr2
Event: DOMSubtreeModified  0, ,  -- null

Opera (Presto):
Event: DOMAttrModified a ADDITION, , one -- attr1
Event: DOMAttrModified a REMOVAL, one,  -- attr1
Event: DOMAttrModified a ADDITION, , two -- attr2

Chrome 48:
Event: DOMSubtreeModified  0, ,  -- null

UIE is not precise here (http://www.w3.org/TR/uievents/#event-type-DOMAttrModified)  but it looks like that setting MutationEvent.relatedNode to new one (when new attr replace old attr) is web compatibile. Other question is what value for MutationEvent.attrChange should be set; 1 (Firefox 43 and IE11) or 2 (Firefox 46.01)

Comment 8

3 years ago
Comment on attachment 8705770 [details] [diff] [review]
part 4.  Change nsDOMAttributeMap::SetNamedItemNS to not remove the existing attribute when there is one

>     if (oldAttr) {
>-      attr = RemoveNamedItem(oldNi, aError);
So in the old code this set attr which is then the return value of the method, as it per spec should be - the old value.


>+      // Just remove it from our hashtable.  This has no side-effects, so we
>+      // don't have to recheck anything after we do it.  Then we'll add our new
>+      // Attr to the hashtable and do the actual attr set on the element.  This
>+      // will make the whole thing look like a single attribute mutation (with
>+      // the new attr node in place) as opposed to a removal and addition.
>+      DropAttribute(oldNi->NamespaceID(), oldNi->NameAtom());
in this new code we don't set attr to any value, which means the method ends up returning null, although it should return the oldAttr
So I would remove RefPtr<Attr> attr; and put RefPtr<Attr> oldAttr; there instead, and then
assign value to oldAttr the same place where we do now, and make the method to return oldAttr.forget() at the end of it.

>+++ b/testing/web-platform/tests/dom/nodes/attributes.html
Is this file doesn't have tests for the return value, please add some.

>+var setAttributeNode_mutation_test = async_test("setAttributeNode, if it fires mutation events, should fire one with the new node when resetting an existing attribute");
>+
>+test(function(){
>+  var el = document.createElement("div")
>+  var attrNode1 = document.createAttribute("foo");
>+  attrNode1.value = "bar";
>+  el.setAttributeNode(attrNode1);
>+  var attrNode2 = document.createAttribute("foo");
>+  attrNode2.value = "baz";
>+
>+  el.addEventListener("DOMAttrModified", function(e) {
>+    // If this never gets called, that's OK, I guess.  But if it gets called, it
>+    // better represent a single modification with attrNode2 as the relatedNode.
>+    // We have to do an inner test() call here, because otherwise the exceptions
>+    // our asserts trigger will get swallowed by the event dispatch code.
>+    setAttributeNode_mutation_test.step(function() {
>+      assert_equals(e.attrName, "foo");
>+      assert_equals(e.attrChange, MutationEvent.MODIFICATION);
>+      assert_equals(e.prevValue, "bar");
>+      assert_equals(e.newValue, "baz");
>+      assert_equals(e.relatedNode, attrNode2);
>+    });
>+  });
>+
>+  el.setAttributeNode(attrNode2);
Like here you could check that return value is attrNode1

>+}, "setAttributeNode, if it fires mutation events, should fire one with the new node when resetting an existing attribute (outer shell)");
>+setAttributeNode_mutation_test.done();
I don't understand this setup here. 
setAttributeNode_mutation_test is created, test() called and then setAttributeNode_mutation_test.done(); run synchronously. What here is async?
Attachment #8705770 - Flags: review?(bugs) → review-

Comment 9

3 years ago
and using also MutationObserver to check what kinds of records are create might be a useful test.
> So in the old code this set attr which is then the return value of the method

Good catch.  We did hit a regression test failure on this too: dom/tests/mochitest/dom-level1-core/test_elementreplaceexistingattributegevalue.html

> Is this file doesn't have tests for the return value, please add some.

It doesn't, and I will.

> What here is async?

My goal was to be able to assert some things in the event listener.  I can't just use the assert_* macros in there, because those simply trigger an exception, which the event dispatch code will then suppress.  Using a nested test() call in there seemed a bit weird to me, and would have caused a situation in which the number of total tests run depends on whether there is mutation event support.  And I believe we're aiming to not have the number of total tests run vary based on stuff like that.

So the async_test bit is a hack around all that: it ensures that the async_test always happens, though if there is no mutation event support it will more or less pass by default.

> and using also MutationObserver to check what kinds of records are create might be a useful test.

Yes; I'm just not up enough on MutationObserver to write such a test quickly.  I can put in the time if you'd like.
Created attachment 8705939 [details] [diff] [review]
Interdiff from part 4 to fix the correctness issue and add a test for it

Please let me know whether there are other things you want me to change in part 4, based on comment 10.
Attachment #8705939 - Flags: review?(bugs)
Comment on attachment 8705939 [details] [diff] [review]
Interdiff from part 4 to fix the correctness issue and add a test for it

This is fine.
Attachment #8705939 - Flags: review?(bugs) → review+

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9543976f8b1c
https://hg.mozilla.org/mozilla-central/rev/f4bbe8741e30
https://hg.mozilla.org/mozilla-central/rev/9ce0b0de338b
https://hg.mozilla.org/mozilla-central/rev/ed50e3e7638d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.