Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 6 users
Status: Fixed
Owner:
Ooo until EOY 2017. Assign no bugs.
Closed: May 25
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux, Windows, Mac
Pri: 2
Type: Bug

Blocking:
issue 725414



Sign in to add a comment
DOMTokenList update steps for classList don't follow the spec
Reported by m.go...@gmail.com, Apr 6 2016 Back to list
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.110 Safari/537.36

Steps to reproduce the problem:
var div = document.createElement('div');
div.className = '   a   b   c   ';
div.classList.add('d');

console.log(div.className); // "   a   b   c   d"

What is the expected behavior?
What is the expected behavior?
`DOMTokenList` update steps' (https://dom.spec.whatwg.org/#domtokenlist) second point is:

"Set an attribute value for the associated element using associated attribute’s local name and the result of running the ordered set serializer for tokens."

For `classList` the "associated attribute's local name" is `class`. The "ordered set serializer" algorithm (https://dom.spec.whatwg.org/#concept-ordered-set-serializer) is:
"The ordered set serializer takes a set and returns the concatenation of the strings in set, separated from each other by U+0020". This means that after updating the context object's classes via the classList API, className value (that reflects the "class" attribute value) should be "a b c d", not "   a   b   c   d".

What went wrong?
The algorithm serializing the context object's classes preserves spaces too much.

Did this work before? No 

Chrome version: 51.0.2700.0  Channel: canary
OS Version: OS X 10.11.4
Flash Version: Shockwave Flash 21.0 r0

It seems to me that the fix for the  issue 526282  was applied only to the `remove` method of DOMTokenList while the normalization needs to be applied on any operations.

The problem applies not only to `add` but also to `remove` that removes a non-existing entry. For example:

var div = document.createElement('div');
div.className = '   a   b   c   ';
div.classList.remove('d');
console.log(div.className); // This should log "a b c" but it logs the original value instead.

Safari has fixed all those issues in its Technical Preview.
 
Comment 1 by m.go...@gmail.com, Apr 6 2016
As far as I understand, `elem.classList.remove('foo')` should also create an empty `class` attribute if it didn't exist before calling this method
Comment 2 by m.go...@gmail.com, Apr 6 2016
This is a followup to  issue 526282  which fixed some parts of the algorithm but not all.
Comment 3 by kojii@chromium.org, Apr 6 2016
Components: -Blink Blink>DOM
Comment 4 by kojii@chromium.org, Apr 6 2016
Labels: OS-All
Labels: Needs-Bisect
Cc: rnimmagadda@chromium.org
Labels: Needs-Feedback
@m.goleb: Could you please provide us the sample test file along with the screen-recording for better understanding, would help us in triaging it further.

Thank you.
Comment 7 by m.go...@gmail.com, Apr 7 2016
I've already provided the test cases. Just paste that to the console:


var div = document.createElement('div');
div.className = '   a   b   c   ';
div.classList.add('d');
console.log(div.className);

And you'll get "   a   b   c   d" instead of the correct "a b c d". The second test case is analogous. What's not clear?
Project Member Comment 8 by sheriffbot@chromium.org, Apr 7 2016
Labels: -Needs-Feedback Needs-Review
Owner: rnimmagadda@chromium.org
Thank you for providing more feedback. Adding requester "rnimmagadda@chromium.org" for another review and adding "Needs-Review" label for tracking.

For more details visit https://sites.google.com/a/chromium.org/dev/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: Hotlist-GoodFirstBug Hotlist-Interop
Owner: dominicc@chromium.org
Status: Available
FWIW the WebKit change might be the one mentioned here:

https://bugs.chromium.org/p/chromium/issues/detail?id=526282#c7

Comment 10 by m.go...@gmail.com, Apr 7 2016
Re #9:

See my comment at https://bugs.webkit.org/show_bug.cgi?id=148589#c17; it seems Safari Technology Preview serializes too aggresively (following the older version of the spec) so either the spec should be changed back or when preparing the patch for Chromium one should take the change from https://github.com/whatwg/dom/issues/105 into account.
Labels: -Needs-Review -OS-All -Needs-Bisect M-50 OS-Linux OS-Windows
Status: Untriaged
Able to repro this issue on Windows 7, MAC (10.11.4) & Ubuntu Trusty (14.04) for Google Chrome Stable Version - 49.0.2623.112

This is a Non-Regression issue existing from M30 - # 30.0.1549.0
49.0.2623.112.png
172 KB View Download
30.0.1549.0.png
108 KB View Download
Status: Available
Would like to work on this.
Comment 14 by tkent@chromium.org, May 15 2016
Owner: ----
Status: ExternalDependency
The specification discussion is ongoing.

Cc: ch.dumez@chromium.org rbyers@chromium.org
tkent@, do you think you'll be doing the Blink code changes on this one, or someone on your team? ch.dumez@ (Apple), rbyers@ and I were discussing interop work in general and this issue came up. It might help resolve the spec-side issue sooner if Blink changed to the hoped-for end state, to be sure that the change will stick.
Comment 17 by tkent@chromium.org, Mar 16 2017
Cc: -ch.dumez@chromium.org
Labels: -M-50
Status: Available
Blockedon: 724906
Owner: tkent@chromium.org
Status: Started
Project Member Comment 20 by bugdroid1@chromium.org, May 23
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/719237584073bc2ef77a64e4f8b1514e66ef037e

commit 719237584073bc2ef77a64e4f8b1514e66ef037e
Author: tkent <tkent@chromium.org>
Date: Tue May 23 07:25:44 2017

DOMTokenList.contains() should not throw.

https://dom.spec.whatwg.org/#dom-domtokenlist-contains-token-token
https://dom.spec.whatwg.org/#dom-domtokenlist-contains
New behavior matches to the DOM standard, Firefox, and Safari.

This CL fixes 60 failing tests in wpt/dom/.

BUG= 600964 

Review-Url: https://codereview.chromium.org/2898063003
Cr-Commit-Position: refs/heads/master@{#473826}

[modify] https://crrev.com/719237584073bc2ef77a64e4f8b1514e66ef037e/third_party/WebKit/LayoutTests/external/wpt/dom/nodes/Element-classlist-expected.txt
[modify] https://crrev.com/719237584073bc2ef77a64e4f8b1514e66ef037e/third_party/WebKit/LayoutTests/fast/dom/HTMLElement/class-list-expected.txt
[modify] https://crrev.com/719237584073bc2ef77a64e4f8b1514e66ef037e/third_party/WebKit/LayoutTests/fast/dom/HTMLElement/class-list-quirks-expected.txt
[modify] https://crrev.com/719237584073bc2ef77a64e4f8b1514e66ef037e/third_party/WebKit/LayoutTests/fast/dom/HTMLElement/resources/class-list.js
[modify] https://crrev.com/719237584073bc2ef77a64e4f8b1514e66ef037e/third_party/WebKit/LayoutTests/fast/dom/HTMLOutputElement/dom-token-list-expected.txt
[modify] https://crrev.com/719237584073bc2ef77a64e4f8b1514e66ef037e/third_party/WebKit/LayoutTests/fast/dom/HTMLOutputElement/dom-token-list.html
[modify] https://crrev.com/719237584073bc2ef77a64e4f8b1514e66ef037e/third_party/WebKit/Source/core/dom/DOMTokenList.cpp
[modify] https://crrev.com/719237584073bc2ef77a64e4f8b1514e66ef037e/third_party/WebKit/Source/core/dom/DOMTokenList.h
[modify] https://crrev.com/719237584073bc2ef77a64e4f8b1514e66ef037e/third_party/WebKit/Source/core/dom/DOMTokenList.idl

Blocking: 725414
Blockedon: -724906
Project Member Comment 23 by bugdroid1@chromium.org, May 24
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1806913f25daa9da5ca69a0115fe74addaa35d32

commit 1806913f25daa9da5ca69a0115fe74addaa35d32
Author: tkent <tkent@chromium.org>
Date: Wed May 24 09:48:00 2017

DOMTokenList: Update serialization algorithm on add()/remove()

We should re-serialize the whole token list even if an operation didn't change
the list items.

* blink::ClassList
 This class doesn't use DOMTokenList::tokens_, and usually uses
 ElementData::class_names_. We can't update it before setAttribute().  So this class
 creates SpaceSplitString for mutation on the fly.

* blink::RelList
 This class doesn't use DOMTokenList::tokens_. MutableSet() should refer to
 rel_values_ instead.

* blink::DOMTokenList
 For other DOMTokenLists, we can mutate tokens_ directly.

BUG= 600964 

Review-Url: https://codereview.chromium.org/2895903002
Cr-Commit-Position: refs/heads/master@{#474229}

[modify] https://crrev.com/1806913f25daa9da5ca69a0115fe74addaa35d32/third_party/WebKit/LayoutTests/external/wpt/dom/nodes/Element-classlist-expected.txt
[delete] https://crrev.com/cc3ecbd519b29c785382cfa0f3fcfe27a483aa01/third_party/WebKit/LayoutTests/external/wpt/dom/nodes/MutationObserver-attributes-expected.txt
[modify] https://crrev.com/1806913f25daa9da5ca69a0115fe74addaa35d32/third_party/WebKit/LayoutTests/fast/dom/HTMLElement/class-list-expected.txt
[modify] https://crrev.com/1806913f25daa9da5ca69a0115fe74addaa35d32/third_party/WebKit/LayoutTests/fast/dom/HTMLElement/class-list-quirks-expected.txt
[modify] https://crrev.com/1806913f25daa9da5ca69a0115fe74addaa35d32/third_party/WebKit/LayoutTests/fast/dom/HTMLElement/resources/class-list.js
[modify] https://crrev.com/1806913f25daa9da5ca69a0115fe74addaa35d32/third_party/WebKit/LayoutTests/fast/dom/HTMLOutputElement/dom-token-list-expected.txt
[modify] https://crrev.com/1806913f25daa9da5ca69a0115fe74addaa35d32/third_party/WebKit/LayoutTests/fast/dom/HTMLOutputElement/dom-token-list.html
[modify] https://crrev.com/1806913f25daa9da5ca69a0115fe74addaa35d32/third_party/WebKit/Source/core/dom/DOMTokenList.cpp
[modify] https://crrev.com/1806913f25daa9da5ca69a0115fe74addaa35d32/third_party/WebKit/Source/core/dom/DOMTokenList.h
[modify] https://crrev.com/1806913f25daa9da5ca69a0115fe74addaa35d32/third_party/WebKit/Source/core/dom/SpaceSplitString.cpp
[modify] https://crrev.com/1806913f25daa9da5ca69a0115fe74addaa35d32/third_party/WebKit/Source/core/html/ClassList.cpp
[modify] https://crrev.com/1806913f25daa9da5ca69a0115fe74addaa35d32/third_party/WebKit/Source/core/html/ClassList.h
[modify] https://crrev.com/1806913f25daa9da5ca69a0115fe74addaa35d32/third_party/WebKit/Source/core/html/RelList.h

Project Member Comment 24 by bugdroid1@chromium.org, May 25
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d5b3e6d930dd8052d1001b52348be1fb0098188

commit 9d5b3e6d930dd8052d1001b52348be1fb0098188
Author: tkent <tkent@chromium.org>
Date: Thu May 25 04:20:04 2017

DOMTokenList should unify duplicated tokens.

The new behavior matches to the DOM standard, Firefox, and Safari.
This CL fixes 85 failing WPT tests.

BUG= 600964 

Review-Url: https://codereview.chromium.org/2903803002
Cr-Commit-Position: refs/heads/master@{#474547}

[delete] https://crrev.com/81064bed1ae35ec03307b6d7ffee8336386429b1/third_party/WebKit/LayoutTests/external/wpt/dom/lists/DOMTokenList-Iterable-expected.txt
[delete] https://crrev.com/81064bed1ae35ec03307b6d7ffee8336386429b1/third_party/WebKit/LayoutTests/external/wpt/dom/lists/DOMTokenList-iteration-expected.txt
[delete] https://crrev.com/81064bed1ae35ec03307b6d7ffee8336386429b1/third_party/WebKit/LayoutTests/external/wpt/dom/lists/DOMTokenList-value-expected.txt
[modify] https://crrev.com/9d5b3e6d930dd8052d1001b52348be1fb0098188/third_party/WebKit/LayoutTests/external/wpt/dom/nodes/Element-classlist-expected.txt
[modify] https://crrev.com/9d5b3e6d930dd8052d1001b52348be1fb0098188/third_party/WebKit/LayoutTests/fast/dom/HTMLElement/class-list-expected.txt
[modify] https://crrev.com/9d5b3e6d930dd8052d1001b52348be1fb0098188/third_party/WebKit/LayoutTests/fast/dom/HTMLElement/class-list-quirks-expected.txt
[modify] https://crrev.com/9d5b3e6d930dd8052d1001b52348be1fb0098188/third_party/WebKit/LayoutTests/fast/dom/HTMLElement/resources/class-list.js
[modify] https://crrev.com/9d5b3e6d930dd8052d1001b52348be1fb0098188/third_party/WebKit/LayoutTests/fast/dom/HTMLOutputElement/dom-token-list-expected.txt
[modify] https://crrev.com/9d5b3e6d930dd8052d1001b52348be1fb0098188/third_party/WebKit/LayoutTests/fast/dom/HTMLOutputElement/dom-token-list.html
[modify] https://crrev.com/9d5b3e6d930dd8052d1001b52348be1fb0098188/third_party/WebKit/LayoutTests/fast/frames/sandboxed-iframe-attribute-parsing-03-expected.txt
[modify] https://crrev.com/9d5b3e6d930dd8052d1001b52348be1fb0098188/third_party/WebKit/Source/core/dom/SpaceSplitString.cpp

Labels: M-60
Status: Fixed
Labels: TE-Verified-M60 TE-Verified-60.0.3112.7
Tested the issue on Mac-10.12.4,Ubuntu-14.04 & windows-7 using chrome dev version#60.0.3112.7 with the steps mentioned in comment #0.Observed that the fix is working as expected. Hence adding TE-Verified labels.

Please find the attached screen cast for the same.
Thanks!!

Win-600964.mp4
613 KB View Download
Sign in to add a comment