HTMLKeygenElement is derived from HTMLKeygenElement. But HTMLOptionElement::ownerSelectElement() only checks the selectTag.
Sorry I meant HTMLKeygenElement is derived from HTMLSelectElement.
Created attachment 30663 [details] fix bug Not sure how to write layout test as Safari uses SelectElement::setSelectedIndex instead of HTMLOptionElement::setSelected for the menu list.
(In reply to comment #2) > Not sure how to write layout test as Safari uses > SelectElement::setSelectedIndex instead of HTMLOptionElement::setSelected for > the menu list. Should be easy. HTMLOptionElement::setSelected can be called directly from JavaScript by setting the value of the selected attribute on the option element.
Comment on attachment 30663 [details] fix bug Fix looks fine. Please add a regression test. Since ownerSelectElement is used in accessKeyAction, index, and setSelected there are multiple ways you could test this simply in a regression test.
Created attachment 30688 [details] layout test file Here is the layout test file. Which directory do you suggest me to put it?
(In reply to comment #5) > Which directory do you suggest me to put it? Thanks for asking. LayoutTests/fast/forms is a good choice. Another possibility would be LayoutTests/fast/dom/HTMLKeygenElement.
Created attachment 30689 [details] new patch file with layout test
Created attachment 30690 [details] remove a tab from the previous layout file
Created attachment 30693 [details] remove another tab from the Changelog
I'd suggest adding "bool isSelectElement(Element*);" to SelectElement.h, and use "isSelectElement" in that function instead of comparing against select/keygen tags. There already is isOptionElement/isOptionGroupElement, and isSelectElement could serve your needs easily. What do you think Darin?
Will land.
Sending LayoutTests/ChangeLog Adding LayoutTests/fast/dom/HTMLKeygenElement Adding LayoutTests/fast/dom/HTMLKeygenElement/keygen-option-select-expected.txt Adding LayoutTests/fast/dom/HTMLKeygenElement/keygen-option-select.html Sending WebCore/ChangeLog Sending WebCore/html/HTMLOptionElement.cpp Transmitting file data ..... Committed revision 44354.
Comment on attachment 30693 [details] remove another tab from the Changelog The enclosed patch does not pass.
Reverting...
What part doesn't the patch pass?
Comment on attachment 30693 [details] remove another tab from the Changelog Re-nominate for review to run through the EWS.
BTW, darin@apple.com marked the patch review+ on 2009-06-01 at 00:38:57 PST. We seemed to have trouble landing it, but we have much better technology now.
Comment on attachment 30693 [details] remove another tab from the Changelog Still looks good. Forwarding Darin Adler's r+.
Comment on attachment 30693 [details] remove another tab from the Changelog Rejecting patch 30693 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 12596 test cases. fast/dom/HTMLKeygenElement/keygen-option-select.html -> failed Exiting early after 1 failures. 5903 tests run. 90.95s total testing time 5902 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 1 test case (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/1569137
Comment on attachment 30693 [details] remove another tab from the Changelog Looks like the problem was that the test doesn't pass.
Created attachment 53493 [details] Proposed patch: Fix the test case
Attachment 53493 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/ChangeLog:14: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 53496 [details] Proposed patch: Fix the test case and the style error of the last patch Fixed the style error of the last patch
Ping? The patch had an r+ originally, the current patch just fixes a style error in the test file. Thanks!
Comment on attachment 53496 [details] Proposed patch: Fix the test case and the style error of the last patch Forwarding r+. Thanks!
Comment on attachment 53496 [details] Proposed patch: Fix the test case and the style error of the last patch Clearing flags on attachment: 53496 Committed r58490: <http://trac.webkit.org/changeset/58490>
All reviewed patches have been landed. Closing bug.