Bug 26016 – HTMLOptionElement::ownerSelectElement() needs to consider keygen

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Grace Kloba]

HTMLKeygenElement is derived from HTMLKeygenElement. But HTMLOptionElement::ownerSelectElement() only checks the selectTag.

[Grace Kloba]

Sorry I meant HTMLKeygenElement is derived from HTMLSelectElement.

[Grace Kloba]

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.

[Darin Adler]

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

[Darin Adler]

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.

[Grace Kloba]

Created attachment 30688 [details]
layout test file

Here is the layout test file. Which directory do you suggest me to put it?

[Darin Adler]

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

[Grace Kloba]

Created attachment 30689 [details]
new patch file with layout test

[Grace Kloba]

Created attachment 30690 [details]
remove a tab from the previous layout file

[Grace Kloba]

Created attachment 30693 [details]
remove another tab from the Changelog

[Nikolas Zimmermann]

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?

[Adam Barth]

Will land.

[Adam Barth]

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.

[Adam Barth]

Comment on attachment 30693 [details]
remove another tab from the Changelog

The enclosed patch does not pass.

[Adam Barth]

Reverting...

[Grace Kloba]

What part doesn't the patch pass?

[Adam Barth]

Comment on attachment 30693 [details]
remove another tab from the Changelog

Re-nominate for review to run through the EWS.

[Adam Barth]

BTW, [e-mail] 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.

[Adam Barth]

Comment on attachment 30693 [details]
remove another tab from the Changelog

Still looks good.  Forwarding Darin Adler's r+.

[WebKit Commit Bot]

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

[Adam Barth]

Comment on attachment 30693 [details]
remove another tab from the Changelog

Looks like the problem was that the test doesn't pass.

[Jan Erik Hanssen]

Created attachment 53493 [details]
Proposed patch: Fix the test case

[WebKit Review Bot]

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.

[Jan Erik Hanssen]

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

[Grace Kloba]

Ping? The patch had an r+ originally, the current patch just fixes a style error in the test file.

Thanks!

[Adam Barth]

Comment on attachment 53496 [details]
Proposed patch: Fix the test case and the style error of the last patch

Forwarding r+.  Thanks!

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.