Bug 14439 - Can't set el.type on a <button> element
: Can't set el.type on a <button> element
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 523.x (Safari 3)
: All All
: P2 Normal
Assigned To:
:
: EasyFix, HasReduction, WebExposed
:
:
  Show dependency treegraph
 
Reported: 2007-06-27 17:51 PST by
Modified: 2012-06-13 11:54 PST (History)


Attachments
test case (220 bytes, text/html)
2007-07-02 05:53 PST, Alexey Proskuryakov
no flags Details
Proposed fix and tests. (8.71 KB, patch)
2012-06-12 11:26 PST, Edaena Salinas
darin: review-
darin: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Proposed fix and tests. (9.46 KB, patch)
2012-06-12 14:59 PST, Edaena Salinas
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2007-06-27 17:51:22 PST
Setting el.type on a button element created with document.createElement() doesn't work. You can set it with el.setAttribute(), however.
------- Comment #1 From 2007-07-02 05:53:50 PST -------
Created an attachment (id=15350) [details]
test case
------- Comment #2 From 2007-07-02 05:55:16 PST -------
Confirmed with r23841.
------- Comment #3 From 2007-07-07 08:48:19 PST -------
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.
------- Comment #4 From 2007-07-07 09:10:50 PST -------
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?
------- Comment #5 From 2007-07-07 10:09:17 PST -------
Yes, the test case passes in Firefox.
------- Comment #6 From 2007-08-10 08:45:21 PST -------
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.
------- Comment #7 From 2012-06-06 14:09:17 PST -------
HTML5 removes the readonly restriction (http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#htmlbuttonelement) so we should too.
------- Comment #8 From 2012-06-12 11:26:36 PST -------
Created an attachment (id=147118) [details]
Proposed fix and tests.
------- Comment #9 From 2012-06-12 11:29:20 PST -------
Please wait for approval from timothy@apple.com (or another member of the Apple Safari Team) before submitting because this patch contains changes to the Apple Mac WebKit.framework public API.
------- Comment #10 From 2012-06-12 11:52:15 PST -------
(From update of attachment 147118 [details])
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.
------- Comment #11 From 2012-06-12 11:52:26 PST -------
Thanks for taking this on!
------- Comment #12 From 2012-06-12 14:59:34 PST -------
Created an attachment (id=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.
------- Comment #13 From 2012-06-12 15:06:04 PST -------
(From update of attachment 147172 [details])
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.
------- Comment #14 From 2012-06-12 17:27:33 PST -------
(From update of attachment 147172 [details])
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.
------- Comment #15 From 2012-06-12 17:33:01 PST -------
(From update of attachment 147172 [details])
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.
------- Comment #16 From 2012-06-12 17:55:50 PST -------
(From update of attachment 147172 [details])
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 + ''".
------- Comment #17 From 2012-06-12 18:00:54 PST -------
(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.
------- Comment #18 From 2012-06-12 21:50:59 PST -------
(From update of attachment 147172 [details])
Clearing flags on attachment: 147172

Committed r120158: <http://trac.webkit.org/changeset/120158>
------- Comment #19 From 2012-06-12 21:51:05 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #20 From 2012-06-13 09:59:29 PST -------
(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]
------- Comment #21 From 2012-06-13 11:54:26 PST -------
(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.