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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 147118: Proposed fix and tests.
https://bugs.webkit.org/attachment.cgi?id=147118&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.