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.

Sign up for GitHub

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

Jump to bottom

<span class="a b c">.classList.replace("c", "a") -> "a b" or "b a"? (DOMTokenList) #442

Closed
ayg opened this issue Apr 18, 2017 · 13 comments · Fixed by #444
Closed

<span class="a b c">.classList.replace("c", "a") -> "a b" or "b a"? (DOMTokenList) #442

ayg opened this issue Apr 18, 2017 · 13 comments · Fixed by #444

Comments

@ayg
Copy link
Contributor

ayg commented Apr 18, 2017

Also filed as w3c#117 because I got confused with the two specs. :(

The spec just says "Replace token in tokens with newToken", but this doesn't actually work in an ordered set, because if you actually replaced it as instructed, the set would have a duplicate entry. Test-case:

data:text/html,<!doctype html>
<script>
var span = document.createElement("span");
span.setAttribute("class", "a b c");
span.classList.replace("c", "a");
document.documentElement.textContent = span.getAttribute("class");
</script>

Firefox Aurora says "a b". Chrome doesn't seem to support .replace(). I don't have Edge or WebKit handy yet to test. In the W3C issue someone reported that WebKit is the same as Firefox.
@annevk
Copy link
Member

annevk commented Apr 18, 2017

I need to rewrite this section on top of the Infra Standard still. Then it'll use https://infra.spec.whatwg.org/#set-replace which does the right thing.

@ayg
Copy link
Contributor Author

ayg commented Apr 18, 2017

That means "b a", right? So it's against implementations in this case?

@annevk
Copy link
Member

annevk commented Apr 18, 2017

Thanks for pointing that out. I think that's a bug we should fix. @domenic do you agree?

@ayg
Copy link
Contributor Author

ayg commented Apr 18, 2017

Do you mean changing the algorithm in infra? Is there a way to check whether any other specs use the "replace" algorithm from infra and make sure we don't break behavior elsewhere with the change? It's a bit hard to test changes to meta-specs like this if you don't have a list of which specs use the algorithms.

@annevk
Copy link
Member

annevk commented Apr 18, 2017

I did mean that, yes. I added it fairly recently in whatwg/infra#103 for something else in DOM that I think guarantees nothing ends up being removed.

@annevk
Copy link
Member

annevk commented Apr 18, 2017 •
edited
Loading

To answer your more general question, we don't have such tooling yet, but I suspect we'll have it in due course as we already have a database for definitions. Shouldn't be too hard to also index all usage of them.

@annevk
Copy link
Member

annevk commented Apr 18, 2017

@ayg did you test "c b a" as input by the way? I could imagine that going the other way.

@ayg
Copy link
Contributor Author

ayg commented Apr 18, 2017 

data:text/html,<!doctype html>
<script>
var span = document.createElement("span");
span.setAttribute("class", "c b a");
span.classList.replace("c", "a");
document.documentElement.textContent = span.getAttribute("class");
</script>

You're right -- in Firefox, this is also "a b". So it seems to do the replace, then keep the first. Anyone want to test WebKit on this second test-case, and Edge on both?

@annevk
Copy link
Member

annevk commented Apr 18, 2017

Safari TP does "b a" for this case.

@annevk
Copy link
Member

annevk commented Apr 18, 2017

Edge does not support replace.

@annevk
Copy link
Member

annevk commented Apr 18, 2017

Safari's implementation is nice in that you either replace or remove the target depending on whether the set contains input. Unfortunately that doesn't quite work with Infra's generic "matching" condition, which allows input such as "replace anything that is not "a" with "x", but I'm not sure if Infra really needs to be like that for replace.

@domenic
Copy link
Member

domenic commented Apr 18, 2017

I'd be up for updating Infra's definition to be more like what Safari does. If necessary we can de-genericize it. We should definitely add an example when we do.

annevk added a commit that referenced this issue Apr 19, 2017
@annevk
Always update a DOMTokenList
d7b281b 
In particular when invoking toggle() and replace(). Also rewrite it in
terms of the Infra Standard.

Fixes #442 and fixes #443.
@annevk annevk mentioned this issue Apr 19, 2017
Merged
annevk added a commit to whatwg/infra that referenced this issue Apr 19, 2017
@annevk
Make set replacement more limited in scope
56e5c0e 
Per discussion in whatwg/dom#442.
@annevk annevk mentioned this issue Apr 19, 2017
Merged
@annevk
Copy link
Member

annevk commented Apr 26, 2017

We ended up aligning Infra with Firefox.

annevk added a commit to whatwg/infra that referenced this issue Apr 26, 2017
@annevk
Make set replacement more limited in scope
68c2896 
Also, replace the first instance of either the item to match or its replacement, rather than just the the item to match.

See also whatwg/dom#442.
annevk added a commit that referenced this issue Apr 26, 2017
@annevk
Always update a DOMTokenList
d396872 
In particular when invoking toggle() and replace(). Also rewrite it in
terms of the Infra Standard.

Fixes #442 and fixes #443.
cdumez added a commit to cdumez/web-platform-tests that referenced this issue Apr 27, 2017
@cdumez
Update Element-classlist.html to cover edge case
579e450 
Update Element-classlist.html to cover edge case at:
- whatwg/dom#442
@cdumez cdumez mentioned this issue Apr 27, 2017
Merged
domenic pushed a commit to web-platform-tests/wpt that referenced this issue Apr 27, 2017
@domenic
Update Element-classlist.html to cover edge case
9236897 
This covers the case discussed at whatwg/dom#442.
hubot pushed a commit to WebKit/WebKit-http that referenced this issue Apr 28, 2017
Update DOMTokenList.replace() to match the latest DOM specification
7d0d78c 
https://bugs.webkit.org/show_bug.cgi?id=171388

Reviewed by Alex Christensen.

LayoutTests/imported/w3c:

Re-sync web-platform-test after:
- web-platform-tests/wpt#5725

This adds test coverage for the behavior change in this patch.

* web-platform-tests/dom/nodes/Element-classlist.html:

Source/WebCore:

Update DOMTokenList.replace() to match the latest DOM specification after:
- whatwg/dom#442
- whatwg/infra#126

The latest spec text is at:
- https://dom.spec.whatwg.org/#dom-domtokenlist-replace
- https://infra.spec.whatwg.org/#set-replace

The behavior change in this patch causes (a, b, c).replace(a, c) to return
(c, b) instead of (b, c). This new behavior is aligned with Firefox as well.

No new tests, updated existing test.

* html/DOMTokenList.cpp:
(WebCore::DOMTokenList::replace):

Source/WTF:

Add Vector::findMatching() API which takes in a lambda function for convenience.
Add optional startIndex parameter to Vector::removeFirstMatching() / removeAllMatching()
to remove items only after given index.

* wtf/Vector.h:
(WTF::minCapacity>::findMatching):
(WTF::minCapacity>::find):
(WTF::minCapacity>::removeFirstMatching):
(WTF::minCapacity>::removeAllMatching):

Tools:

Add API test coverage for new Vector API.

* TestWebKitAPI/Tests/WTF/Vector.cpp:
(TestWebKitAPI::TEST):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@215943 268f45cc-cd09-0410-ab3c-d52691b4dbfc
annevk pushed a commit to web-platform-tests/wpt that referenced this issue Aug 9, 2017
@ayg @annevk
Editorial: remove outdated comment
9830c5a 
whatwg/dom#442 was resolved a long time ago.
rachelandrew pushed a commit to rachelandrew/web-platform-tests that referenced this issue Nov 8, 2017
@ayg @rachelandrew
Editorial: remove outdated comment
e6cb712 
whatwg/dom#442 was resolved a long time ago.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Always update a DOMTokenList whatwg/dom
3 participants
@domenic @ayg @annevk