Changes in the whitespace and duplicates handling in the DOMTokenList specification

RESOLVED DUPLICATE of bug 869788

Status

()

Core
DOM: Core & HTML
RESOLVED DUPLICATE of bug 869788
5 years ago
2 years ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

22 Branch
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.

Comment 1

5 years ago
FWIW, that's correct.

Comment 2

5 years ago
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(' ')`.
(Assignee)

Comment 3

5 years ago
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 ?

Comment 4

5 years ago
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.

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
(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.

Comment 7

5 years ago
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.
(Assignee)

Comment 8

5 years ago
Okay, let's make a patch for the current state of the spec only then.

Comment 9

5 years ago
(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.

Comment 10

5 years ago
Yes, this is the wrong forum. Please take it to www-dom@w3.org.
(Assignee)

Updated

5 years ago
Assignee: nobody → felash
(Assignee)

Comment 11

5 years ago
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 ?

Comment 12

5 years ago
Assumptions? Are you not reading the specification? Looks correct to me.
(Assignee)

Comment 13

5 years ago
I was reading the specification, but wanted to double-check. It's easy to miss things from the specification.
Keywords: dev-doc-needed
(Assignee)

Comment 14

5 years ago
Created attachment 799788 [details] [diff] [review]
wip patch v1

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 :)
(Assignee)

Comment 15

5 years ago
Created attachment 807816 [details] [diff] [review]
patch v1

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.
(Assignee)

Updated

5 years ago
Attachment #799788 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
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)

Updated

5 years ago
Flags: needinfo?(bugs)
(Assignee)

Comment 17

5 years ago
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).
(Assignee)

Comment 19

5 years ago
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?
(Assignee)

Comment 21

5 years ago
spiritRKS1910> this will actually be fixed by this bug ;)
as you see, it's not fixed yet !
(Assignee)

Updated

5 years ago
Summary: Changes in the whitespace handling in the DOMTokenList specification → Changes in the whitespace and duplicates handling in the DOMTokenList specification
(Assignee)

Comment 22

5 years ago
PR for the web-platform-tests repository is at https://github.com/julienw/web-platform-tests/pull/1
(Assignee)

Comment 23

5 years ago
The actual PR is https://github.com/w3c/web-platform-tests/pull/360, thanks Msg2er for creating it, I really messed up ;)
(Assignee)

Comment 24

4 years ago
Created attachment 817754 [details] [diff] [review]
patch v2

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

Updated

4 years ago
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+

Updated

4 years ago
Flags: needinfo?(Ms2ger)
(Assignee)

Comment 26

4 years ago
Created attachment 820298 [details] [diff] [review]
patch v3

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.
(Assignee)

Comment 28

3 years ago
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?
(Assignee)

Comment 30

3 years ago
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?
(Assignee)

Comment 33

3 years ago
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: → bug 869788
This should be covered by bug 869788.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 869788
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.