[webkit-reviews] review granted: [Bug 14439] Can't set el.type on a <button> element : [Attachment 147172] Proposed fix and tests.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Edaena Salinas <edaena at gmail.com>'s
request for review:
Bug 14439: Can't set el.type on a <button> element
https://bugs.webkit.org/show_bug.cgi?id=14439

Attachment 147172: Proposed fix and tests.
https://bugs.webkit.org/attachment.cgi?id=147172&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=147172&action=review


> Source/WebCore/html/HTMLButtonElement.cpp:56
> +void HTMLButtonElement::setType(const String& type)

We’ll get slightly better efficiency if we make the type of this argument be
AtomicString. But we can do that after this lands in a separate patch.

> LayoutTests/fast/dom/HTMLButtonElement/change-type.html:61
> +    shouldBe("btn.type = null; btn.type", "'submit'");
> +    shouldBe("btn.getAttribute('type')", "null + ''");

The better way to write the expected value here is just "'null'", since it’s
just the string null that we expect. "null + ''" is a roundabout way to write
that.

But I am surprised this is the correct behavior. It might be worth looking more
closely at the HTML specification and the other web browsers to double check. I
would have expected that setting type to null would delete the type attribute,
not set it to the string "null", and if so we can achieve that by setting the
right flag in the IDL file.

> LayoutTests/fast/dom/HTMLButtonElement/change-type.html:64
> +    shouldBe("btn.type = undefined; btn.type", "'submit'");
> +    shouldBe("btn.getAttribute('type')", "undefined + ''");

As with null, "'undefined'" would be better than "undefined + ''".