RESOLVED FIXED 89030
el.getAttribute('type') returns 'null' when setting el.type to null
https://bugs.webkit.org/show_bug.cgi?id=89030
Summary el.getAttribute('type') returns 'null' when setting el.type to null
Edaena Salinas
Reported 2012-06-13 12:27:06 PDT
Created attachment 147388 [details] test After setting the type of a button to null using btn.type = null, btn.getAttribute('type') should return a JavaScript null and not the string 'null'.
Attachments
test (227 bytes, text/html)
2012-06-13 12:27 PDT, Edaena Salinas
no flags
Proposed fix and updated tests. (4.54 KB, patch)
2012-06-13 14:31 PDT, Edaena Salinas
darin: review+
Proposed fix and updated tests. (5.39 KB, patch)
2012-06-13 15:14 PDT, Edaena Salinas
no flags
Edaena Salinas
Comment 1 2012-06-13 14:31:17 PDT
Created attachment 147413 [details] Proposed fix and updated tests.
Alexey Proskuryakov
Comment 2 2012-06-13 14:39:10 PDT
Comment on attachment 147413 [details] Proposed fix and updated tests. View in context: https://bugs.webkit.org/attachment.cgi?id=147413&action=review > Source/WebCore/ChangeLog:14 > + * html/HTMLButtonElement.cpp: > + (WebCore::HTMLButtonElement::setType): > + * html/HTMLButtonElement.h: > + (HTMLButtonElement): > + * html/HTMLButtonElement.idl: This is a good place to explain changes in your files. For example, the change from String to AtomicString definitely needs an explanation, because it's not even part of fixing this bug. E.g. "Address another unrelated review comment from bug 14439." > LayoutTests/fast/dom/HTMLButtonElement/change-type.html:64 > shouldBe("btn.getAttribute('type')", "undefined + ''"); As you are addressing unrelated feedback, you can consider changing this to "'undefined'", too.
Alexey Proskuryakov
Comment 3 2012-06-13 14:39:42 PDT
> E.g. "Address another unrelated review comment from bug 14439." Better yet, "Address another unrelated review comment from bug 14439 for slightly better performance."
Darin Adler
Comment 4 2012-06-13 14:52:55 PDT
Comment on attachment 147413 [details] Proposed fix and updated tests. View in context: https://bugs.webkit.org/attachment.cgi?id=147413&action=review >> LayoutTests/fast/dom/HTMLButtonElement/change-type.html:64 >> shouldBe("btn.getAttribute('type')", "undefined + ''"); > > As you are addressing unrelated feedback, you can consider changing this to "'undefined'", too. I agree, you should do that.
Darin Adler
Comment 5 2012-06-13 14:59:26 PDT
Comment on attachment 147413 [details] Proposed fix and updated tests. For what it’s worth, I agree with all of Alexey’s comments and suggest you post a new patch dealing with them.
Edaena Salinas
Comment 6 2012-06-13 15:14:51 PDT
Created attachment 147424 [details] Proposed fix and updated tests.
Alexey Proskuryakov
Comment 7 2012-06-13 15:42:50 PDT
Comment on attachment 147424 [details] Proposed fix and updated tests. + Reviewed by NOBODY (OOPS!). For minor changes like these, you could have edited ChangeLogs to say "Reviewed by Darin Adler", and post a new patch with "commit-queue?" flag alone, no "review?" flag. + (HTMLButtonElement): Changed parameter to be AtomicString to address an unrelated + review comment from bug 14439 for slightly better performance. We traditionally just say "Ditto" for repeated comments.
WebKit Review Bot
Comment 8 2012-06-14 01:31:43 PDT
Comment on attachment 147424 [details] Proposed fix and updated tests. Clearing flags on attachment: 147424 Committed r120296: <http://trac.webkit.org/changeset/120296>
WebKit Review Bot
Comment 9 2012-06-14 01:31:48 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.