Bug 14439 – Can't set el.type on a <button> element

• Component: DOM
• Status: RESOLVED FIXED

[Kravvitz]

Setting el.type on a button element created with document.createElement() doesn't work. You can set it with el.setAttribute(), however.

[Alexey Proskuryakov]

Created attachment 15350 [details]
test case

[Alexey Proskuryakov]

Confirmed with r23841.

[David Kilzer (:ddkilzer)]

This is not a regression as the same bug occurs in Safari 2.0.4 (419.3) with original WebKit on Mac OS X 10.4.10.

type is a readonly attribute according to <http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-34812697>. Do other browsers allow you to change it?

[Alexey Proskuryakov]

Yes, the test case passes in Firefox.

[Darin Adler]

This is very easy to fix by changing HTMLButtonElement.h/cpp/idl to add setType, that just sets the type attribute.

If we decide to make the change, we should also tell the HTML 5 folks, since this behavior conflicts with the DOM Level 2 standard, so we can standardize on Firefox's behavior.

[Sam Weinig]

HTML5 removes the readonly restriction (http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#htmlbuttonelement) so we should too.

[Edaena Salinas]

Created attachment 147118 [details]
Proposed fix and tests.

[WebKit Review Bot]

Please wait for approval from [e-mail] (or another member of the Apple Safari Team) before submitting because this patch contains changes to the Apple Mac WebKit.framework public API.

[Darin Adler]

Comment on attachment 147118 [details]
Proposed fix and tests.

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

> Source/WebCore/html/HTMLButtonElement.cpp:67
> +void HTMLButtonElement::setType(const String& type)
> +{
> +    if (type.lower() == "reset")
> +        m_type = RESET;
> +    else if (type.lower() == "submit")
> +        m_type = SUBMIT;
> +    else if (type.lower() == "button")
> +        m_type = BUTTON;
> +    else
> +        m_type = SUBMIT;
> +    
> +}

This is probably the wrong way to do it. The right way for this to work is for the setter half of this to be straight content attribute reflection. This is explained in the HTML standard in the section that talks about content attribute that are enumerated attributes and IDL attributes are limited to only known values. The getter is special but the setter is just straight content attribute reflection. Since I believe there is no IDL syntax to make the setter reflect and the getter work with custom code, you will need to write a standard reflection setter function. That looks like this:

void HTMLButtonElement::setType(const AtomicString& type)
{
    setAttribute(typeAttr, type);
}

And we need test cases showing that getAttribute can see the actual value string the type attribute was set to and can distinguish strings even though the type attribute will return only one of the three valid type strings.

If you look at the HTMLInputElement::setType you’ll see an example of something almost exactly like this, only with an unneeded special case for empty string.

Later we could add IDL syntax for setter-only reflection and then it would autogenerate the setter and we could delete the function.

If we were keeping this function, we should use equalIgnoringCase rather than lower, and we should not have a case for submit.

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

There is no need to set the type twice. This code does it outside the shouldBe expression and then a second time inside the shouldBe expression. Only the one inside the shouldBe is needed.

As I mentioned above we need to test both what btn.type returns and also what btn.getAttribute('type') returns.

[Darin Adler]

Thanks for taking this on!

[Edaena Salinas]

Created attachment 147172 [details]
Proposed fix and tests.

I am not sure about the behavior of getAttribute('type') since it is not returning the same value as el.type. It does not return a lowercase valid value or an invalid value reseted to 'submit'.  

In addition, when setting btn.type = null, btn.getAttribute('type') returns '' in Firefox. In WebKit it returns null + '' (null of type string).  Because of this, the corresponding test will fail in Firefox.

[Edaena Salinas]

Comment on attachment 147172 [details]
Proposed fix and tests.

I'm not sure about the behavior of getAttribute("type"). It does not ignore uppercase letters of valid inputs such as 'RESET' and it does not return 'submit' when invalid values were set using el.type.

In addition, when setting btn.type = null, btn.getAttribute('type') returns '' in Firefox. In WebKit it returns null + ''. The corresponding test for this fails in Firefox.

[Jon Lee]

Comment on attachment 147172 [details]
Proposed fix and tests.

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

> Source/WebCore/html/HTMLButtonElement.idl:33
> +        attribute DOMString type;

I believe with the [Reflect] extended attribute added here you don't need setType() on HTMLButtonElement.{h,cpp}, since the bindings generator will automatically create it.

[Jon Lee]

Comment on attachment 147172 [details]
Proposed fix and tests.

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

>> Source/WebCore/html/HTMLButtonElement.idl:33
>> +        attribute DOMString type;
> 
> I believe with the [Reflect] extended attribute added here you don't need setType() on HTMLButtonElement.{h,cpp}, since the bindings generator will automatically create it.

Ach! I missed Darin's comment, please ignore.
I would, however, file a new bug to extend the bindings generator to allow for custom get/set with reflection, and cite this bug.

[Darin Adler]

Comment on attachment 147172 [details]
Proposed fix and tests.

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 + ''".

[Darin Adler]

(In reply to comment #13)
> I'm not sure about the behavior of getAttribute("type"). It does not ignore uppercase letters of valid inputs such as 'RESET'

That is correct behavior.

> it does not return 'submit' when invalid values were set using el.type.

That is also correct behavior.

> In addition, when setting btn.type = null, btn.getAttribute('type') returns '' in Firefox. In WebKit it returns null + ''. The corresponding test for this fails in Firefox.

Yes, I think we have a bug here, and it’s worth fixing it.

We have to nail down what the expected result really is. I would not expect the empty string, though, I’d expect a JavaScript null.

Then, to get that behavior, we would need to add something like [TreatNullAs=NullString] to the attribute in the IDL file; not sure I have the syntax right. This would pass through the JavaScript null as a null string, which setAttribute would handle by removing the attribute.

[WebKit Review Bot]

Comment on attachment 147172 [details]
Proposed fix and tests.

Clearing flags on attachment: 147172

Committed r120158: <http://trac.webkit.org/changeset/120158>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Edaena Salinas]

(In reply to comment #17)

> Then, to get that behavior, we would need to add something like [TreatNullAs=NullString] to the attribute in the IDL file; not sure I have the syntax right. This would pass through the JavaScript null as a null string, which setAttribute would handle by removing the attribute.

Does this also apply to undefined? [TreatUndefinedAs=NullString]

[Darin Adler]

(In reply to comment #20)
> (In reply to comment #17)
> > Then, to get that behavior, we would need to add something like [TreatNullAs=NullString] to the attribute in the IDL file; not sure I have the syntax right. This would pass through the JavaScript null as a null string, which setAttribute would handle by removing the attribute.
> 
> Does this also apply to undefined?

I believe it does not. I think that undefined is supposed to change into the string "undefined". But that’s from memory. Might want to research or test with other browsers to be 100% sure.