Bug 16798

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.

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

Created attachment 18433 [details] Proposed patch

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 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.

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 on attachment 18601 [details] Almost working patch You really shouldn't be subclassing attributeChanged.

Created attachment 18799 [details] test case

Taking bug. I have a new patch for it.

Created attachment 18964 [details] Patch to fix the bug I will land the excellent test case in the bug as a layout test.

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 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

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.

Yeah that is why it's still open.

Fixed.