Closed Bug 1236258 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

40 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: crimsteam, Assigned: bzbarsky)

Details

Attachments

(5 files)

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
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: nobody → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8705767 - Flags: review?(bugs) → review+
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+
Attachment #8705769 - Flags: review?(bugs) → review+
(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 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-
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.
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+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.