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

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect)

Description

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

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.

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

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.

Comment 2

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

<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

[Olli Pettay [:smaug][[e-mail]]]

Note, MutationEvents != MutationObserver.
This bug is about DOMTokenList::toggle not generating MutationRecords for MutationObserver, right?

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

(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

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

> // 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.