RESOLVED FIXED 16798
<button> does not have type=submit (Acid3 bug)
https://bugs.webkit.org/show_bug.cgi?id=16798
Summary <button> does not have type=submit (Acid3 bug)
Eric Seidel (no email)
Reported 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; },
Attachments
Proposed patch (5.73 KB, patch)
2008-01-14 02:42 PST, Andrew Wellington
darin: review-
Almost working patch (6.17 KB, patch)
2008-01-22 03:01 PST, Andrew Wellington
hyatt: review-
test case (530 bytes, text/html)
2008-01-30 13:36 PST, Robert Blaut
no flags
Patch to fix the bug (1.41 KB, patch)
2008-02-06 11:53 PST, Dave Hyatt
darin: review+
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
Eric Seidel (no email)
Comment 1 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.
Eric Seidel (no email)
Comment 2 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.
Andrew Wellington
Comment 3 2008-01-14 02:42:49 PST
Created attachment 18433 [details] Proposed patch
Eric Seidel (no email)
Comment 4 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>.
Darin Adler
Comment 5 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.
Andrew Wellington
Comment 6 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.
Dave Hyatt
Comment 7 2008-01-22 09:12:35 PST
Comment on attachment 18601 [details] Almost working patch You really shouldn't be subclassing attributeChanged.
Robert Blaut
Comment 8 2008-01-30 13:36:09 PST
Created attachment 18799 [details] test case
Dave Hyatt
Comment 9 2008-02-06 11:49:16 PST
Taking bug. I have a new patch for it.
Dave Hyatt
Comment 10 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.
Eric Seidel (no email)
Comment 11 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.
Darin Adler
Comment 12 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
Eric Seidel (no email)
Comment 13 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.
Dave Hyatt
Comment 14 2008-02-08 14:58:07 PST
Yeah that is why it's still open.
Dave Hyatt
Comment 15 2008-02-11 12:22:44 PST
Fixed.
Note You need to log in before you can comment on or make changes to this bug.