Bug 186883

Summary: Implement support for Element.toggleAttribute
Product: WebKit Reporter: Jonathan Kingston <jkingston>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cmarcelo, commit-queue, darin, dbates, esprehn+autocc, ews-watchlist, ggaren, kangil.han, kondapallykalyan, rniwa, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=187347
Attachments:
Description Flags
Patch none

Description Jonathan Kingston 2018-06-21 02:07:30 PDT 
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
Comment 1 Chris Dumez 2018-07-03 12:04:51 PDT 
Created attachment 344202 [details]
Patch
Comment 2 WebKit Commit Bot 2018-07-03 13:46:45 PDT 
Comment on attachment 344202 [details]
Patch

Clearing flags on attachment: 344202

Committed r233475: <https://trac.webkit.org/changeset/233475>
Comment 3 WebKit Commit Bot 2018-07-03 13:46:46 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 4 Radar WebKit Bug Importer 2018-07-03 13:47:21 PDT 
<rdar://problem/41790122>
Comment 5 Darin Adler 2018-07-04 12:08:48 PDT 
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?
Comment 6 Chris Dumez 2018-07-05 08:55:57 PDT 
(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.
Comment 7 Chris Dumez 2018-07-05 08:56:46 PDT 
(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);
Comment 8 Chris Dumez 2018-07-05 09:08:13 PDT 
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"
Comment 9 Chris Dumez 2018-07-05 09:11:51 PDT 
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.
Comment 10 Chris Dumez 2018-07-05 09:49:34 PDT 
(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