Closed
Bug 910121
Opened 11 years ago
Closed 9 years ago
Changes in the whitespace and duplicates handling in the DOMTokenList specification
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
DUPLICATE
of bug 869788
People
(Reporter: julienw, Assigned: julienw)
References
()
Details
Attachments
(1 file, 3 obsolete files)
19.03 KB,
patch
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
FWIW, that's correct.
Comment 2•11 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•11 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•11 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•11 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•11 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•11 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•11 years ago
|
||
Okay, let's make a patch for the current state of the spec only then.
Comment 9•11 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•11 years ago
|
||
Yes, this is the wrong forum. Please take it to www-dom@w3.org.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → felash
Assignee | ||
Comment 11•11 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•11 years ago
|
||
Assumptions? Are you not reading the specification? Looks correct to me.
Assignee | ||
Comment 13•11 years ago
|
||
I was reading the specification, but wanted to double-check. It's easy to miss things from the specification.
Updated•11 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 14•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
Attachment #799788 -
Attachment is obsolete: true
Assignee | ||
Comment 16•11 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•11 years ago
|
Flags: needinfo?(bugs)
Assignee | ||
Comment 17•11 years ago
|
||
discussed this over IRC, we need to change the tests in https://github.com/w3c/web-platform-tests first.
Comment 18•11 years ago
|
||
(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•11 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.
Comment 20•11 years ago
|
||
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•11 years ago
|
||
spiritRKS1910> this will actually be fixed by this bug ;) as you see, it's not fixed yet !
Assignee | ||
Updated•11 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•11 years ago
|
||
PR for the web-platform-tests repository is at https://github.com/julienw/web-platform-tests/pull/1
Assignee | ||
Comment 23•11 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•11 years ago
|
||
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•11 years ago
|
Flags: needinfo?(Ms2ger)
Comment 25•11 years ago
|
||
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•11 years ago
|
Flags: needinfo?(Ms2ger)
Assignee | ||
Comment 26•11 years ago
|
||
Changed the stringifier as well. Try is https://tbpl.mozilla.org/?tree=Try&rev=e33eb31310db
Attachment #817754 -
Attachment is obsolete: true
Comment 27•9 years ago
|
||
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•9 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) !
Comment 29•9 years ago
|
||
Isn't this a duplicate of bug 869788?
Assignee | ||
Comment 30•9 years ago
|
||
Ah yes, likely.
Comment 31•9 years ago
|
||
(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.
Comment 32•9 years ago
|
||
(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•9 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: → 869788
Comment 34•9 years ago
|
||
This should be covered by bug 869788.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Updated•9 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•