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

Future for unique tokens in DOMTokenList #201

Closed
ArkadiuszMichalski opened this issue Mar 31, 2016 · 17 comments
Closed

Future for unique tokens in DOMTokenList #201

ArkadiuszMichalski opened this issue Mar 31, 2016 · 17 comments

Comments

@ArkadiuszMichalski
Copy link
Contributor

ArkadiuszMichalski commented Mar 31, 2016

Per DOM tokens in DOMTokenList should be unique, which is provided by ordered set parser. But after long time already beyond this is not implemented by any engine, or mabye I wrong so please correct me (Edge, Webkit or Servo do this?).
Some bugs from Mozilla:
https://bugzilla.mozilla.org/show_bug.cgi?id=869788
https://bugzilla.mozilla.org/show_bug.cgi?id=910121

Repeated tokens cause that other definition around DOMTokenList are not valid, for example invoking add() should run the "update steps" (what means set associated attr to new value taken from ordered set serializer for tokens, so this attr's value should have unique tokens too). But browsers don't do this and we get repeated tokens. In the other site for remove() we get better behaviour (but not in all engine).

<script>

    var el = document.createElement("div");

    el.className = " a a b ";
    console.log(el.className.length); // 7
    console.log(el.classList.length); // 3
    console.log(Array.prototype.slice.call(el.classList).join(" ")); // "a a b"

    el.classList.add("a"); // should remove duplicate and white space in associated attr value

    console.log(el.className.length); // 7
    console.log(el.classList.length); // 3
    console.log(Array.prototype.slice.call(el.classList).join(" ")); // "a a b"

    el.classList.remove("a");

    console.log(el.className.length); // 1 (Gecko and Presto return 2, we have "b ")
    console.log(el.classList.length); // 1
    console.log(Array.prototype.slice.call(el.classList).join(" ")); // "b"

</script>

Unique tokens in DOMTokenList still are considered as something attainable in the near future?

@mgol
Copy link

mgol commented Apr 6, 2016

FWIW, Safari Technology Preview has fixed all those bugs and is now following the spec.

EDIT: See #105 (comment) though, they are serializing on regular stringification as well.

@mgol
Copy link

mgol commented Apr 6, 2016

This is what I get in Safari Tech Preview for this test case:

var el = document.createElement("div");

el.className = " a a b ";
console.log(el.className.length); // 7
console.log(el.classList.length); // 2
console.log(Array.prototype.slice.call(el.classList).join(" ")); // "a b"

el.classList.add("a"); // should remove duplicate and white space in associated attr value

console.log(el.className.length); // 3
console.log(el.classList.length); // 2
console.log(Array.prototype.slice.call(el.classList).join(" ")); // "a b"

el.classList.remove("a");

console.log(el.className.length); // 1
console.log(el.classList.length); // 1
console.log(Array.prototype.slice.call(el.classList).join(" ")); // "b"

So it seems to be serializing very aggressively.

@ArkadiuszMichalski
Copy link
Contributor Author

Still something is wrong, console.log(el.className.length); // 3 << what value is here "a a b"? Per DOM, after invoke add() should be "a b" because this method run "update steps" (https://dom.spec.whatwg.org/#concept-dtl-update).

@mgol
Copy link

mgol commented Apr 6, 2016

Still something is wrong, console.log(el.className.length); // 3 << what value is here a a b? Per DOM, after invoke add() should be a b.

It's a b. There are 3 characters there, don't forget the space. :-)

@ArkadiuszMichalski
Copy link
Contributor Author

Oh right, sorry, so it's correct:). Have you access to Edge, what result we get on this engine?

@cdumez
Copy link

cdumez commented Apr 7, 2016

Yes, WebKit trunk always drops duplicates and cleans up whitespace currently. It looks like I need to change so that we only do this on add/remove/toggle/update/replace?

@cvrebert
Copy link
Member

cvrebert commented Apr 8, 2016

Have you access to Edge, what result we get on this engine?

Edge 13.10586:

var el = document.createElement("div");

el.className = " a a b ";
console.log(el.className.length); // 7
console.log(el.classList.length); // 3
console.log(Array.prototype.slice.call(el.classList).join(" ")); // "a a b"

el.classList.add("a"); // should remove duplicate and white space in associated attr value

console.log(el.className.length); // 7
console.log(el.classList.length); // 3
console.log(Array.prototype.slice.call(el.classList).join(" ")); // "a a b"

el.classList.remove("a");

console.log(el.className.length); // 1
console.log(el.classList.length); // 1
console.log(Array.prototype.slice.call(el.classList).join(" ")); // "b"

@ArkadiuszMichalski
Copy link
Contributor Author

@mgol Can you report what Safari Tech Preview product for this (commetns represent value from Firefox and Chrome):

<script>
    var el = document.createElement("div");
    el.classList = " a a b ";
    console.log(el.className.length); // 7
    console.log(el.classList.length); // 3
    console.log(el.attributes[0].value); // " a a b "
    console.log(Array.prototype.slice.call(el.classList).join(" ")); // "a a b"
</script>

@mgol
Copy link

mgol commented Apr 10, 2016

@ArkadiuszMichalski

var el = document.createElement("div");
el.classList = " a a b ";
console.log(el.className.length); // 3
console.log(el.classList.length); // 2
console.log(el.attributes[0].value); // "a b"
console.log(Array.prototype.slice.call(el.classList).join(" ")); // "a b"

@ArkadiuszMichalski
Copy link
Contributor Author

So, per DOM, it's wrong: setting classList should [PutForwards=value] and directly set class content attribute to passing value " a a b ", but here I see "a b". But at least tokens in set are correct.
https://dom.spec.whatwg.org/#dom-domtokenlist-value
This behaviour fianlly cames from merging DOMSettableTokenList into DOMTokenList #119

@mgol
Copy link

mgol commented Apr 10, 2016

@ArkadiuszMichalski Please read my post on GitHub, I pasted some things wrong and edited it immediately, you see the old thing when you look at the mail.

@ArkadiuszMichalski
Copy link
Contributor Author

I use GitHub but I had to reload the page, ok, so I removed this "last space" question.

@cdumez
Copy link

cdumez commented Apr 10, 2016

@ArkadiuszMichalski : Why is the behavior wrong?

el.classList = " a a b ";
should be equivalent to calling:
el.classList.value = "a a b ";

shouldn't it?

in which case, el.attributes[0].value should probably return "a b".

If there is a bug, I don't mind fixing it but this looks like expected behavior to me.

Well, that is before the following change:
2920fc1

Which we are trying to get reverted via:
#105 (comment)

At least, this is my interpretation of:
http://heycam.github.io/webidl/#PutForwards

@ArkadiuszMichalski
Copy link
Contributor Author

@cdumez
Yes, el.classList on setting is equivalent el.classList.value on setting, and DOM definition:
"Setting the value attribute must set an attribute value for the associated element using associated attribute’s local name and the given value."
I thought that in #105 you are focusing only on stringifier for DOMTokenList, but yes, this fix 2920fc1 change DOMSettableTokenList.value too (but only getting, setting was change earlier, here e48ced4). This change was done probably because that was like stable browsers work (which implemented DOMSettableTokenList for some content attribute, now replaced by DOMTokenList).

Changing stringifier for DOMTokenList doesn't need to enforce changing DOMTokenList.value . They don't need have to the same behavior, it all depends on what is more expected by the developers
and compatible with actual content. Setting asociated attr through DOMTokenList to orginal passing value also can be useful.

Recently a lot of changes were made around sets and hard to follow them, especially when browsers still operate in their own way. The current definition doesn't look bad and really, if others don't follow the new changes, it makes no sense to modify anything in DOM once again.

@annevk
Copy link
Member

annevk commented Aug 17, 2016

Okay to mark this as a duplicate of #105?

@ArkadiuszMichalski
Copy link
Contributor Author

ArkadiuszMichalski commented Aug 20, 2016

@annevk If #105 also fix (clarify) this case #201 (comment) then you can close. Due #201 (comment) Safari Technology Preview make progress here, but still not correct per spec.. I still look implementation of this at least in one engine in Windows (even in dev version). Observing the progress of work in this area I wondering why you want change DOMTokenList stringifier, now DOM describe behaviour of Edge/IE/Blink/Gecko vs alone Webkit, so what change requires less effort?

@annevk
Copy link
Member

annevk commented Mar 16, 2017

@ArkadiuszMichalski I'm folding this into #105, if I'm overlooking something let me know, but my plan is to file bugs against all browsers apart from WebKit and see how that shakes out. Also, going forward I'll make sure (and you should remind me) to include tests for normative changes so we don't end up in this situation again. It's clearly not great and apologies for that.

@annevk annevk closed this as completed Mar 16, 2017
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

5 participants