Supported property names for NamedNodeMap #141

ArkadiuszMichalski · 2016-01-04T06:46:29Z

https://dom.spec.whatwg.org/#dom-namednodemap-item
"A NamedNodeMap object’s supported property names, all unenumerable, are the qualified names of the attributes in the attribute list, in order."

But these supported property names must be all qualified names of the attributes in the attribute list? So for example we can have five attributes with qualified name a:test and different namespace and all of them belong to supported property names? If yes then what Object.getOwnPropertyNames() and DevTools should return (when inspect NamedNodeMap object), only first (or last) unique qualified name or all qualified names (including duplicates)?

Small testcase:

<!DOCTYPE html>
<html>
<body>
    <script>

        var newP = document.createElement("P");
        newP.setAttributeNS("www.test1.com", "a:new", "Test1");
        newP.setAttributeNS("www.test2.com", "a:new", "Test2");
        newP.setAttributeNS("www.test3.com", "a:new", "Test3");
        newP.setAttributeNS("www.test4.com", "A:new", "Test4");
        newP.setAttributeNS("www.test5.com", "A:new", "Test5");
        newP.setAttributeNS("www.test6.com", "A:new", "Test6");

        console.log(newP.attributes);
        console.log(Object.getOwnPropertyNames(newP.attributes));
        console.log(newP.attributes["a:new"]);
        console.log(newP.attributes["A:new"]);

    </script>
</body>
</html>

The text was updated successfully, but these errors were encountered:

annevk · 2016-01-04T08:50:33Z

Per the current definition you would get duplicates, but that does not seem to be what browsers do. @bzbarsky, should I just add a statement to exclude duplicates here?

Note that Chrome does not even list the property names for getOwnPropertyNames()... Might be a known bug though.

annevk · 2016-01-04T08:51:26Z

(That a:new and A:new give the first attribute back is not surprising by the way. p is in the HTML namespace and therefore getNamedItem() lowercases.)

ArkadiuszMichalski · 2016-01-04T09:25:24Z

Yep, this a:new and A:new is not surprising. More interested is situation when supported property names contains duplicates (it is acceptable?) and what should be done if contains. In DOM for HTMLCollection we have always unique value, I check HTML and in most case we have unique value (but not always). Web IDL specification is very economical in their descriptions: https://heycam.github.io/webidl/#dfn-supported-property-names

Edit: BZ noticed earlier that Chrome has bug for NamedNodeMap #104 (comment)

bzbarsky · 2016-01-04T17:02:08Z

@bzbarsky, should I just add a statement to exclude duplicates here?

Probably good enough, yes, as long as order is clearly defined. Certainly that's all Gecko does in practice. See https://hg.mozilla.org/mozilla-central/file/d7a0ad85d9fb/dom/base/nsDOMAttributeMap.cpp#l219 for the implementation as of today.

Might be a known bug though.

There are all sorts of bugs around getOwnPropertyNames on stuff with named getters in browsers. Gecko didn't have proper support for it on NamedNodeMap until Firefox 45, for example. Whether it's known is harder to tell. Probably worth just reporting.

annevk · 2016-01-04T17:52:14Z

So I didn't know why we wanted to change this, but from https://tc39.github.io/ecma262/#sec-invariants-of-the-essential-internal-methods it seems clear enough. Only property keys observable through [[GetOwnProperty]] are to be returned in the List of [[OwnPropertyKeys]]. Now I'm wondering though, how can A:new be observed? It seems it will always be lowercased, so should it be in the "List" @bzbarsky?

bzbarsky · 2016-01-04T18:02:48Z

The list should in theory include all names which return something other than undefined for Object.getOwnPropertyDescriptor, right?

This actually kinda sucks; due to the lowercasing behavior this means that all possible uppercasings should be in the list, or something. That seems fairly undesirable, not to mention probably not so web-compatible.

Now the good news is that since named props can't be made non-configurable and objects with such props can't be made non-extensible there isn't actually a violation of the spec section you cite here if we just have [[OwnPropertyKeys]] return some subset of the things that will return useful stuff from [[GetOwnProperty]]. It's slightly weird behavior, but not inherently contradicting the ES spec.

annevk · 2016-01-04T18:13:03Z

Sounds about right. Paging @bterlson and @domenic so they can verify.

domenic · 2016-01-05T02:28:41Z

I think we would also need to add something that ensures these objects cannot be made non-extensible, as otherwise you could do Object.preventExtensions(o) and that would trigger the

If the object is non-extensible, the returned List must contain only the keys of all own properties of the object that are observable using [[GetOwnProperty]].

clause. (That clause is admittedly a bit ambiguous; it seems like it's trying to place an upper bound, but the phrasing "all own properties" seems to imply a lower bound too.)

It looks like making properties non-configurable is guarded against already in the platform object [[DefineOwnProperty]] definition.

If this is taken care of then I agree there is no invariant violation (just a slightly-annoying mismatch).

annevk · 2016-01-05T08:14:52Z

Object.preventExtensions() does indeed throw per IDL.

domenic · 2016-01-05T08:21:39Z

Ah, I see, it's just defined by monkeypatching Object.preventExtensions and friends instead of by overriding the appropriate internal method. Someone should fix that…

ArkadiuszMichalski · 2016-01-06T02:12:11Z

I don't have deep knowlage around all ECMAScript stuff so I have to ask, in this example (test in Firefox):

<script>

    var newP = document.createElement("P");
    newP.setAttributeNS("www.test1.com", "A:new", "Test1");

    console.log(newP.attributes); // NamedNodeMap [ A:new="Test1" ]
    console.log(Object.getOwnPropertyNames(newP.attributes)); // Array [ "0", "A:new" ]
    console.log(newP.attributes.hasOwnProperty("A:new")); // false
    console.log(newP.attributes["A:new"]); // undefined
    console.log(Object.getOwnPropertyDescriptor(newP.attributes, "A:new")); // undefined

</script>

Returning "A:new" by getOwnPropertyNames() when other methods return false/undefined is correct/intend?

bzbarsky · 2016-01-06T02:47:30Z

No, that seems bogus. Looks like getOwnPropertyNames should only return lowercased versions of names, right?

ArkadiuszMichalski · 2016-01-06T03:17:54Z

Or maybe skip them when we have element in HTML namespace and owner is HTML Document (I mean all attr's names what have minimum one capital letter because, what I see now, hasOwnProperty, [] and getOwnPropertyDescriptor always make lowercasing). I don't know what will be correct in ES side.

This all exist because using setAttributeNS() for element in HTML namespace and owner is HTML Document. In other cases it is more predictable.

<script>

    var newP = document.createElementNS("", "P");
    newP.setAttributeNS("www.test1.com", "A:new", "Test1");

    console.log(newP.attributes); // NamedNodeMap [ A:new="Test1" ]
    console.log(Object.getOwnPropertyNames(newP.attributes)); // Array [ "0", "A:new" ]
    console.log(newP.attributes.hasOwnProperty("A:new")); // true
    console.log(newP.attributes["A:new"]); // A:new="Test1"
    console.log(Object.getOwnPropertyDescriptor(newP.attributes, "A:new")); // Object { value: A:new="Test1", writable: false, enumerable: false, configurable: true }

</script>

annevk · 2016-01-06T11:24:42Z

If we return the lowercased version of the attribute, it would still be wrong, since ele.attributes.hasOwnProperty("a:new") would still return false and newP.attributes["a:new"] would still be undefined (lowercasing only happens on the input side).

I think what we need to do is check NamedNodeMap's associated element for being in the HTML namespace and its associated document being an HTML document. Then for each attribute we lowercase it, and if it's no longer equal to itself, we ignore it (we could also check for an "uppercase" code point, but we don't have that primitive at the moment). Apart from that we keep the requirements around removing duplicates and using attribute list order.

This will require changes to Gecko as currently it does return uppercase attribute names. It will also require changes to other browser for unrelated changes. Once we fix this we should also file a bug against wpt.

Does that make sense @bzbarsky?

bzbarsky · 2016-01-07T04:42:15Z

Then for each attribute we lowercase it, and if it's no longer equal to itself, we ignore it

Ah, indeed. That makes sense, yes.

annevk · 2016-01-07T12:14:38Z

Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1237580.

bzbarsky · 2016-01-14T02:18:33Z

Note that I'll add a web platform test in the Gecko bug.

annevk closed this as completed in 5dbefd1 Jan 5, 2016

annevk reopened this Jan 6, 2016

annevk closed this as completed in 8898ac5 Jan 7, 2016

yongsheng mentioned this issue Jan 7, 2017

NamedNodeMap: remove duplicates and omit low-case property names w3c/dom#28

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supported property names for NamedNodeMap #141

Supported property names for NamedNodeMap #141

ArkadiuszMichalski commented Jan 4, 2016

annevk commented Jan 4, 2016

annevk commented Jan 4, 2016

ArkadiuszMichalski commented Jan 4, 2016

bzbarsky commented Jan 4, 2016

annevk commented Jan 4, 2016

bzbarsky commented Jan 4, 2016

annevk commented Jan 4, 2016

domenic commented Jan 5, 2016

annevk commented Jan 5, 2016

domenic commented Jan 5, 2016

ArkadiuszMichalski commented Jan 6, 2016

bzbarsky commented Jan 6, 2016

ArkadiuszMichalski commented Jan 6, 2016

annevk commented Jan 6, 2016

bzbarsky commented Jan 7, 2016

annevk commented Jan 7, 2016

bzbarsky commented Jan 14, 2016

Supported property names for NamedNodeMap #141

Supported property names for NamedNodeMap #141

Comments

ArkadiuszMichalski commented Jan 4, 2016

annevk commented Jan 4, 2016

annevk commented Jan 4, 2016

ArkadiuszMichalski commented Jan 4, 2016

bzbarsky commented Jan 4, 2016

annevk commented Jan 4, 2016

bzbarsky commented Jan 4, 2016

annevk commented Jan 4, 2016

domenic commented Jan 5, 2016

annevk commented Jan 5, 2016

domenic commented Jan 5, 2016

ArkadiuszMichalski commented Jan 6, 2016

bzbarsky commented Jan 6, 2016

ArkadiuszMichalski commented Jan 6, 2016

annevk commented Jan 6, 2016

bzbarsky commented Jan 7, 2016

annevk commented Jan 7, 2016

bzbarsky commented Jan 14, 2016