WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug