Closed Bug 910121 Opened 7 years ago Closed 5 years ago

Changes in the whitespace and duplicates handling in the DOMTokenList specification

Categories

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

22 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 869788

People

(Reporter: julienw, Assigned: julienw)

References

()

Details

Attachments

(1 file, 3 obsolete files)

The DOMTokenList seems to imply we should remove the spaces starting or ending the underlying string attribute, when we change it using the token list object.
FWIW, that's correct.
Not just starting and ending whitespace should be removed, whitespace _between_ tokens should be replaced to exactly one space character when accessing property like `className`.

In fact, property like `className` should just reflect a list of tokens binded to an element and should be generated dynamically by just doing something like `Array.join(' ')`.
Anne> thanks 

Marat> that, we already do it :)

But we should not change className if we don't use classList at all, right ? Eg: merely setting className with weird whitespace patterns, and getting it, should we get the same value ?

I understand that as soon as we use classList.add, .toggle, .remove, then we should remove leading and traling whitespace and "compress" "insiders" whitespace, even if .remove arguments are not currently in className. Am I right ?
Actually, className always reflects the class attribute so should have the same whitespace on getting and setting.

Manipulating classList manipulates the class attribute by serializing the internal token set using a space as separator.
(In reply to comment #3)
> But we should not change className if we don't use classList at all, right ?
> Eg: merely setting className with weird whitespace patterns, and getting it,
> should we get the same value ?

Historically (and probably implementations-wise) true, though I would prefer to change behavior (and spec) so that `className` would always (regadless of whether `classList` has been used to manipulate it) just reflect the class list (such change would unlikely break anything in the Web).

But at least manipulating `classList` property should lead to removing redundant whitespace from `class` attribute and `className` property (same applies to other `tokenList`s) as Anne said.
(In reply to Marat Tanalin | tanalin.com from comment #5)

> Historically (and probably implementations-wise) true, though I would prefer
> to change behavior (and spec) so that `className` would always (regadless of
> whether `classList` has been used to manipulate it) just reflect the class
> list (such change would unlikely break anything in the Web).
> 

I perfectly agree with that (and that's how I supposed it worked back when I discovered classList).

Therefore you suggest that basically classList should be the authoritative property, and className (and the class attribute) would reflect classList, so basically the contrary of what's done right now.
The class attribute cannot reflect classList at least until something is modified. Given that className has been reflecting the class attribute for ages, I doubt that change would be compatible.
Okay, let's make a patch for the current state of the spec only then.
(In reply to Julien Wajsberg [:julienw] from comment #6)
Exactly.

(In reply to Anne (:annevk) from comment #7)
As I've mentioned above, I'm not sure such compatibility, while makes sense in general, does really matter as for whitespace in token-list strings.

Probably here is not a quite appropriate place for such proposals, but it probably makes sense to spec a _new_ DOM property like `tokenList.asString` or a method like `tokenList.toString()` that would always and predictably return normalized (without beginning and ending whitepace and with exactly one space as token separator) form of token-list value string.

Maybe this would be the best option in terms of usability, flexibility and backward compatibility.
Yes, this is the wrong forum. Please take it to www-dom@w3.org.
Assignee: nobody → felash
What should be our behavior regarding code like this:

elt.className = "a a a";
elt.classList.add("b");
=> "a b";

elt.className = "a a a";
elt.classList.add("a");
=> "a";

elt.className = "a a a";
elt.classList.remove("b");
=> "a";

Am I correct with these assumptions ?
Assumptions? Are you not reading the specification? Looks correct to me.
I was reading the specification, but wanted to double-check. It's easy to miss things from the specification.
Attached patch wip patch v1 (obsolete) — Splinter Review
WIP patch

I rewrote AddInternal and RemoveInternal to use arrays instead of string, which permits easier dupe check.

Tests are passing, but removing duplicates in GetAsArray in nsAttrValue doesn't feel good, and I'll change this. That's why I'm not asking review yet.

But I wanted to share progress :)
Attached patch patch v1 (obsolete) — Splinter Review
So, I rewrote the whole feature using arrays instead of string manipulations, trying to follow Boris Zbarsky's request in bug 814014 comment 13.

My try is in https://tbpl.mozilla.org/?tree=Try&rev=0a3bbe15ed66

I'm afraid of string copies in some places, but I don't really know how to do differently.
Attachment #799788 - Attachment is obsolete: true
The old Opera tests are failing (dom/imptests/html/old-tests/submission/Opera/microdata), but this is actually because of the spec change.

Should I remove, or update those tests ?
Flags: needinfo?(bugs)
Flags: needinfo?(bugs)
discussed this over IRC, we need to change the tests in https://github.com/w3c/web-platform-tests first.
(In reply to Julien Wajsberg [:julienw] from comment #11)

You ask only for duplicates, but what about the order?

elt.className = "a c";
elt.classList.add("b");
=> "a b c"; 
or
=> "a c b"; 

I ask because all actual browser return second output and wonder if I correctly understand term "ordered" in DOM spec (and it should be first).
Be careful, "ordered" does not mean "sorted".

"ordered" means the set has a particular order, it means if you have "a c b" at first, it will stay "a c b" in the end.
Thx for clear explanation, there was something incomprehensible to me so I finally asked.

And about comment #11, I guess that examples shows the correct action (duplicate should be removed). So why actual Firofox don't do this now (I test last nightly). I read on other bug that you make some work on add() and remove() method (the possibility of using a variable number of arguments), but duplicates in ordered set of tokens for DOMTokenList object still exist. It's bug or I understood something wrong again?
spiritRKS1910> this will actually be fixed by this bug ;)
as you see, it's not fixed yet !
Summary: Changes in the whitespace handling in the DOMTokenList specification → Changes in the whitespace and duplicates handling in the DOMTokenList specification
PR for the web-platform-tests repository is at https://github.com/julienw/web-platform-tests/pull/1
The actual PR is https://github.com/w3c/web-platform-tests/pull/360, thanks Msg2er for creating it, I really messed up ;)
Attached patch patch v2 (obsolete) — Splinter Review
Changes requested over IRC.

New try is https://tbpl.mozilla.org/?tree=Try&rev=57a85ad38f4b

The DOM tests still fail because the new ones were not imported in the tree yet. I guess they will be imported with "todo" and I'll need to change to "ok" in this patch ?
Attachment #807816 - Attachment is obsolete: true
Flags: needinfo?(Ms2ger)
https://hg.mozilla.org/mozilla-central/rev/182b8450e32f

See the .json files in there; you should be able to remove the relevant lines when you land.
Flags: in-testsuite+
Flags: needinfo?(Ms2ger)
Attached patch patch v3Splinter Review
Changed the stringifier as well.

Try is https://tbpl.mozilla.org/?tree=Try&rev=e33eb31310db
Attachment #817754 - Attachment is obsolete: true
Any plans to finish/land this patch, is still in onboard? I see that for some method like toggle() or remove() white spaces and duplicates are removed for ordered set of tokens but only when match to passing argument (including actualization for class attribute), but even for this two methods is not 100% correct (per DOM) because invoking this methods without any argument nothing updates (but should and it's easy way to clean up any duplicate).

Now Chrome 43 and IE 11 do the same thing like FX (to be more precise - IE not support optional argument). If DOMTokenList interface gain more popularity any changes in future may be more difficult.
It's clearly not on my plate (I'm not in this team and was doing this in my free time). I didn't think it was that important for the web that's why I did other things in my free time.

If someone want to take it and finish it, feel free to steal my patch (assuming it still applies, which is unlikely) !
Isn't this a duplicate of bug 869788?
Ah yes, likely.
(In reply to Michał Gołębiowski [:m_gol] from comment #29)
> Isn't this a duplicate of bug 869788?

Bug 869788 covers only stringifier behaviour, here probably touch all cases.
(In reply to Arkadiusz Michalski (Spirit) from comment #31)
> (In reply to Michał Gołębiowski [:m_gol] from comment #29)
> > Isn't this a duplicate of bug 869788?
> 
> Bug 869788 covers only stringifier behaviour, here probably touch all cases.

So maybe this bug should be marked as blocked by bug 869788? Or bug 869788 can be set as a duplicate of this one?
Not really a big deal IMO. I link them with the "See also" property but no need to worry too much about it :)
See Also: → 869788
This should be covered by bug 869788.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 869788
You need to log in before you can comment on or make changes to this bug.