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

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kravvitz 2007-06-27 17:51:22 PDT
Setting el.type on a button element created with document.createElement() doesn't work. You can set it with el.setAttribute(), however.
Comment 1 Alexey Proskuryakov 2007-07-02 05:53:50 PDT
Created attachment 15350 [details]
test case
Comment 2 Alexey Proskuryakov 2007-07-02 05:55:16 PDT
Confirmed with r23841.
Comment 3 David Kilzer (:ddkilzer) 2007-07-07 08:48:19 PDT
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 mitz@webkit.org 2007-07-07 09:10:50 PDT
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 Alexey Proskuryakov 2007-07-07 10:09:17 PDT
Yes, the test case passes in Firefox.
Comment 6 Darin Adler 2007-08-10 08:45:21 PDT
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 Sam Weinig 2012-06-06 14:09:17 PDT
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 Edaena Salinas 2012-06-12 11:26:36 PDT
Created attachment 147118 [details]
Proposed fix and tests.
Comment 9 WebKit Review Bot 2012-06-12 11:29:20 PDT
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 Darin Adler 2012-06-12 11:52:15 PDT
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.
Comment 11 Darin Adler 2012-06-12 11:52:26 PDT
Thanks for taking this on!
Comment 12 Edaena Salinas 2012-06-12 14:59:34 PDT
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.
Comment 13 Edaena Salinas 2012-06-12 15:06:04 PDT
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.
Comment 14 Jon Lee 2012-06-12 17:27:33 PDT
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.
Comment 15 Jon Lee 2012-06-12 17:33:01 PDT
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.
Comment 16 Darin Adler 2012-06-12 17:55:50 PDT
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 + ''".
Comment 17 Darin Adler 2012-06-12 18:00:54 PDT
(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 WebKit Review Bot 2012-06-12 21:50:59 PDT
Comment on attachment 147172 [details]
Proposed fix and tests.

Clearing flags on attachment: 147172

Committed r120158: <http://trac.webkit.org/changeset/120158>
Comment 19 WebKit Review Bot 2012-06-12 21:51:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Edaena Salinas 2012-06-13 09:59:29 PDT
(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 Darin Adler 2012-06-13 11:54:26 PDT
(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.