Bug 16798 – <button> does not have type=submit (Acid3 bug)

• Component: DOM
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Test 59: FAIL (expected button, got:  - <button> doesn't have type=button)

    function () {
      // test 59: attributes of <button> elements
      var button = document.createElement('button');
      assertEquals(button.type, "button", "<button> doesn't have type=button");
      button.setAttribute("type", "submit");
      assertEquals(button.type, "submit", "<button type=submit> doesn't have type=submit");
      button.removeAttribute("type");
      assertEquals(button.type, "button", "<button> doesn't have type=button back");
      button.setAttribute('value', 'apple');
      button.appendChild(document.createTextNode('banana'));
      assertEquals(button.value, 'apple', "wrong button value");
      return 4;
    },

[Eric Seidel (no email)]

Currently the code is:

const AtomicString& HTMLButtonElement::type() const
{
    return getAttribute(typeAttr);
}

http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-27430092

seems to imply it's always "button".

We just need to make a test case which creates an HTMLButtonElement, checks the "button" attribute and dom property tries to change each, and makes sure that it doesn't change.

[Eric Seidel (no email)]

I guess Hixie and I both read the spec wrong, it should start out as "submit".  Hixie has corrected the test.

[Andrew Wellington]

Created attachment 18433 [details]
Proposed patch

[Eric Seidel (no email)]

Comment on attachment 18433 [details]
Proposed patch

I guess overriding attributeChanged is the right course of action.  Normally we use parseMappedAttribute, but this attribute can't be mapped (but that might be OK to use so long as mapAttribute() returns eNone) I'd have to ask hyatt or darin which design is preferred.

Your test case needs to check .getAttribute("type") after each set.  We also need to run your test case under FF and IE to make sure we don't horribly disagree with both.  It's OK to bring us closer to the spec here, but we might push back against Hixie if IE and FF already agree on this behavior.

Thanks ever so much for the patch!

It might also be interesting to test another valid input type, like "text", that wouldn't be valid for <button>.

[Darin Adler]

Comment on attachment 18433 [details]
Proposed patch

+void HTMLButtonElement::attributeChanged(Attribute* attr, bool preserveDecls)
+{
+    if (attr->name() == typeAttr && !attr->value())
+        m_type = SUBMIT;
+    HTMLGenericFormElement::attributeChanged(attr, preserveDecls);
+}

We should not add this override. Instead the code for this should just go into parseMappedAttribute. It should just be rewritten so it sets m_type to SUBMIT any time the value is neither RESET nor BUTTON.

I'd like to confirm that HTMLButtonElement.type really returns a different value than getAttribute("type") in the other browsers. Accordingly, the test case should check both type and getAttribute("type").

+debug('<span>Start test 1</span>');

I don't think these are great. We normally don't include markup in the debug statements, and I think it's better not to. And I don't think it adds much to the output to just number tests here. Maybe if the messages said things like "removing type attribute" it would be better.

[Andrew Wellington]

Created attachment 18601 [details]
Almost working patch

This patch works *except* that it causes the following layout tests to fail:
fast/dom/wrapper-classes.html 
platform/mac/fast/dom/wrapper-classes-objc.html

With an assertion failure:
ASSERTION FAILED: !m_deletionHasBegun
(/Users/andrew/Documents/WebKit/WebCore/platform/TreeShared.h:56 void WebCore::TreeShared<T>::ref() [with T = WebCore::Node])

I have no idea why, except removing setAttribute in the HTMLButtonElement constructor makes it go away.

[Dave Hyatt]

Comment on attachment 18601 [details]
Almost working patch

You really shouldn't be subclassing attributeChanged.

[Robert Blaut]

Created attachment 18799 [details]
test case

[Dave Hyatt]

Taking bug.  I have a new patch for it.

[Dave Hyatt]

Created attachment 18964 [details]
Patch to fix the bug

I will land the excellent test case in the bug as a layout test.

[Eric Seidel (no email)]

Created attachment 18965 [details]
more complete (untested) test case (drop in fast/dom/HTMLButtonElement/resources/)

This test case is probably wrong, but at least it exercises more cases.  The values it tests for can be changed by someone who knows how this should behave.

[Darin Adler]

Comment on attachment 18964 [details]
Patch to fix the bug

+    return emptyAtom;

I think there should be an ASSERT_NOT_REACHED here. Also, I think it should return "submit" rather than the empty string. But since it should never be reached that doesn't really matter.

r=me

[Eric Seidel (no email)]

Hyatt landed this in:
http://trac.webkit.org/projects/webkit/changeset/30095
but with no tests!

I guess we'll leave this open until he lands the test cases too.

[Dave Hyatt]

Yeah that is why it's still open.

[Dave Hyatt]

Fixed.