nsDOMTokenList::toggle does not fire mutation event if classList is unchanged

RESOLVED INVALID

Status

()

RESOLVED INVALID
3 years ago
3 years ago

People

(Reporter: wcpan, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox42 affected)

Details

nsDOMTokenList::AddInternal and nsDOMTokenList::RemoveInternal will always set element's attribute, regardless it needs to be changed or not.

i.e.
if you use element.classList.add() to add an existing class or element.classList.remove() to remove a non-existing class, it will trigger relevant events.
Summary: Avoid modify attributes if nsDOMTokenList does not change → Avoid modifing attributes if nsDOMTokenList does not change
Summary: Avoid modifing attributes if nsDOMTokenList does not change → Avoid modifying attributes if nsDOMTokenList does not change
The live spec says: https://dom.spec.whatwg.org/#interface-domtokenlist

The add(tokens...) method must run these steps:
 ...
 3. For each token in tokens, in given order, that is not in tokens, append token to tokens.
 4. Run the update steps.

A DOMTokenList object’s update steps
are:
  ...
  2. 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.

So the attribute should always be assigned to the result of serializer for tokens. The question is whether assigning classList same value should trigger updates.
Summary: Avoid modifying attributes if nsDOMTokenList does not change → nsDOMTokenList::toggle does not fire mutation event
Summary: nsDOMTokenList::toggle does not fire mutation event → nsDOMTokenList::toggle does not fire mutation event if classList is unchanged
<div id="test" class="test-1">
</div>


var div = document.querySelector('#test');
var observer = new MutationObserver(function (mutations) {
    mutations.forEach(function (mutation) {
        console.info(mutation);
    });
});
var out = document.querySelector('#out');
observer.observe(div, {
    attributes: true,
});

// will trigger attribute change event
div.classList.add('test-1');
// will trigger attribute change event
div.classList.remove('test-2');

// will NOT trigger attribute change event
div.classList.toggle('test-1', true);
// will NOT trigger attribute change event
div.classList.toggle('test-2', false);

Comment 3

3 years ago
Note, MutationEvents != MutationObserver.
This bug is about DOMTokenList::toggle not generating MutationRecords for MutationObserver, right?
(In reply to Olli Pettay [:smaug] from comment #3)
> Note, MutationEvents != MutationObserver.
> This bug is about DOMTokenList::toggle not generating MutationRecords for
> MutationObserver, right?

Yes. We found it is inconsistent with add/remove method. Note that our add/remove are also behave differently with google chrome.

Comment 5

3 years ago
Which way differently to chrome? At some point, IIRC, the spec changed and chrome wasn't updated.

Comment 6

3 years ago
> // will trigger attribute change event
> div.classList.add('test-1');
> // will trigger attribute change event
> div.classList.remove('test-2');
> 
> // will NOT trigger attribute change event
> div.classList.toggle('test-1', true);
So we enter the following
If token is in tokens, run these substeps:
    -If force is either not passed or is false, then remove token from tokens, run the update steps, and return false.
    -Otherwise, return true.
and force is true so we don't remove anything nor run update steps. So, nothing should happen.

 

> // will NOT trigger attribute change event
> div.classList.toggle('test-2', false);
Otherwise, run these substeps:
    If force is passed and is false, return false.
    Otherwise, append token to tokens, run the update steps, and return true. 

test-2 isn't in classlist, and false is passed, we just return false.



So I don't any us doing anything against the spec.


Please reopen if I missed something.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.