Bug 186883 – Implement support for Element.toggleAttribute

• Component: DOM
• Status: RESOLVED FIXED

[Jonathan Kingston]

Element.toggleAttribute provides a simple boolean interface the same as classList.toggle for Element.

See https://github.com/whatwg/dom/issues/461

Firefox implementation bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1469592

Other browsers are willing to implement: https://github.com/whatwg/dom/issues/461#issuecomment-398206390

[Chris Dumez]

Created attachment 344202 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 344202 [details]
Patch

Clearing flags on attachment: 344202

Committed r233475: <https://trac.webkit.org/changeset/233475>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41790122>

[Darin Adler]

Comment on attachment 344202 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344202&action=review

> Source/WebCore/dom/Element.h:117
> +    ExceptionOr<bool> toggleAttribute(const AtomicString& name, std::optional<bool> force);

Should this be localName instead of name?

> Source/WebCore/dom/Element.idl:47
> +    [CEReactions, MayThrowException] boolean toggleAttribute(DOMString qualifiedName, optional boolean force);

Should this be localName instead of qualifiedName?

[Chris Dumez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 344202 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344202&action=review
> 
> > Source/WebCore/dom/Element.h:117
> > +    ExceptionOr<bool> toggleAttribute(const AtomicString& name, std::optional<bool> force);
> 
> Should this be localName instead of name?
> 
> > Source/WebCore/dom/Element.idl:47
> > +    [CEReactions, MayThrowException] boolean toggleAttribute(DOMString qualifiedName, optional boolean force);
> 
> Should this be localName instead of qualifiedName?

We try to keep the IDL in sync with the spec and the spec says "qualifiedName".
I will check what makes sense for the implementation though.

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> (In reply to Darin Adler from comment #5)
> > Comment on attachment 344202 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=344202&action=review
> > 
> > > Source/WebCore/dom/Element.h:117
> > > +    ExceptionOr<bool> toggleAttribute(const AtomicString& name, std::optional<bool> force);
> > 
> > Should this be localName instead of name?
> > 
> > > Source/WebCore/dom/Element.idl:47
> > > +    [CEReactions, MayThrowException] boolean toggleAttribute(DOMString qualifiedName, optional boolean force);
> > 
> > Should this be localName instead of qualifiedName?
> 
> We try to keep the IDL in sync with the spec and the spec says
> "qualifiedName".
> I will check what makes sense for the implementation though.

At the very least, the new method is consistent with the others:
    WEBCORE_EXPORT const AtomicString& getAttribute(const AtomicString& name) const;
    WEBCORE_EXPORT ExceptionOr<void> setAttribute(const AtomicString& name, const AtomicString& value);

[Chris Dumez]

Comment on attachment 344202 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344202&action=review

> Source/WebCore/dom/Element.cpp:1284
> +    unsigned index = elementData() ? elementData()->findAttributeIndexByName(caseAdjustedLocalName, false) : ElementData::attributeNotFound;

Specification says the input is a qualifiedName so I should probably be using the findAttributeIndexByName(QualifiedName) overload here. I'll look into this further.

> Source/WebCore/dom/Element.cpp:1287
> +            setAttributeInternal(index, QualifiedName { nullAtom(), caseAdjustedLocalName, nullAtom() }, emptyString(), NotInSynchronizationOfLazyAttribute);

Here is correct though because the spec says:
"If force is not given or is true, create an attribute whose local name is qualifiedName"

[Chris Dumez]

Comment on attachment 344202 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344202&action=review

> LayoutTests/imported/w3c/web-platform-tests/dom/nodes/attributes.html:134
> +  el.toggleAttribute("foo:bar");

Actually the behavior for qualified name is tested here and we pass this test. findAttributeIndexByName(const AtomicString&) does the right thing when the attribute has a prefix:
attribute.name().toString() == caseAdjustedName

Based on this, I believe I should rename localName / name in the implementation to qualifiedName, to match the IDL and the spec.

[Chris Dumez]

(In reply to Chris Dumez from comment #9)
> Comment on attachment 344202 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344202&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/dom/nodes/attributes.html:134
> > +  el.toggleAttribute("foo:bar");
> 
> Actually the behavior for qualified name is tested here and we pass this
> test. findAttributeIndexByName(const AtomicString&) does the right thing
> when the attribute has a prefix:
> attribute.name().toString() == caseAdjustedName
> 
> Based on this, I believe I should rename localName / name in the
> implementation to qualifiedName, to match the IDL and the spec.

https://bugs.webkit.org/show_bug.cgi?id=187347