Bug 16798 - <button> does not have type=submit (Acid3 bug)
Summary: <button> does not have type=submit (Acid3 bug)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Dave Hyatt
URL: http://acid3.acidtests.org/
Keywords: HasReduction
Depends on:
Blocks: Acid3
  Show dependency treegraph
 
Reported: 2008-01-09 00:18 PST by Eric Seidel (no email)
Modified: 2008-02-11 12:22 PST (History)
1 user (show)

See Also:


Attachments
Proposed patch (5.73 KB, patch)
2008-01-14 02:42 PST, Andrew Wellington
darin: review-
Details | Formatted Diff | Diff
Almost working patch (6.17 KB, patch)
2008-01-22 03:01 PST, Andrew Wellington
hyatt: review-
Details | Formatted Diff | Diff
test case (530 bytes, text/html)
2008-01-30 13:36 PST, Robert Blaut
no flags Details
Patch to fix the bug (1.41 KB, patch)
2008-02-06 11:53 PST, Dave Hyatt
darin: review+
Details | Formatted Diff | Diff
more complete (untested) test case (drop in fast/dom/HTMLButtonElement/resources/) (1.47 KB, application/x-javascript)
2008-02-06 12:09 PST, Eric Seidel (no email)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-01-09 00:18:54 PST
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;
    },
Comment 1 Eric Seidel (no email) 2008-01-09 00:39:01 PST
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.
Comment 2 Eric Seidel (no email) 2008-01-12 19:57:08 PST
I guess Hixie and I both read the spec wrong, it should start out as "submit".  Hixie has corrected the test.
Comment 3 Andrew Wellington 2008-01-14 02:42:49 PST
Created attachment 18433 [details]
Proposed patch
Comment 4 Eric Seidel (no email) 2008-01-14 09:45:07 PST
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>.
Comment 5 Darin Adler 2008-01-14 10:58:26 PST
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.
Comment 6 Andrew Wellington 2008-01-22 03:01:46 PST
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.
Comment 7 Dave Hyatt 2008-01-22 09:12:35 PST
Comment on attachment 18601 [details]
Almost working patch

You really shouldn't be subclassing attributeChanged.
Comment 8 Robert Blaut 2008-01-30 13:36:09 PST
Created attachment 18799 [details]
test case
Comment 9 Dave Hyatt 2008-02-06 11:49:16 PST
Taking bug.  I have a new patch for it.
Comment 10 Dave Hyatt 2008-02-06 11:53:37 PST
Created attachment 18964 [details]
Patch to fix the bug

I will land the excellent test case in the bug as a layout test.
Comment 11 Eric Seidel (no email) 2008-02-06 12:09:57 PST
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.
Comment 12 Darin Adler 2008-02-07 13:38:47 PST
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
Comment 13 Eric Seidel (no email) 2008-02-08 14:27:46 PST
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.
Comment 14 Dave Hyatt 2008-02-08 14:58:07 PST
Yeah that is why it's still open.

Comment 15 Dave Hyatt 2008-02-11 12:22:44 PST
Fixed.