Bug 26016 - HTMLOptionElement::ownerSelectElement() needs to consider keygen
: HTMLOptionElement::ownerSelectElement() needs to consider keygen
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-25 19:58 PDT by Grace Kloba
Modified: 2010-04-29 02:33 PDT (History)
6 users (show)

See Also:


Attachments
fix bug (1.22 KB, patch)
2009-05-25 20:21 PDT, Grace Kloba
darin: review-
Details | Formatted Diff | Diff
layout test file (687 bytes, text/html)
2009-05-26 17:20 PDT, Grace Kloba
no flags Details
new patch file with layout test (3.68 KB, patch)
2009-05-26 18:05 PDT, Grace Kloba
no flags Details | Formatted Diff | Diff
remove a tab from the previous layout file (3.68 KB, patch)
2009-05-26 18:23 PDT, Grace Kloba
no flags Details | Formatted Diff | Diff
remove another tab from the Changelog (3.69 KB, patch)
2009-05-26 20:06 PDT, Grace Kloba
abarth: review-
commit-queue: commit‑queue-
Details | Formatted Diff | Diff
Proposed patch: Fix the test case (4.10 KB, patch)
2010-04-15 17:21 PDT, Jan Erik Hanssen
no flags Details | Formatted Diff | Diff
Proposed patch: Fix the test case and the style error of the last patch (4.11 KB, patch)
2010-04-15 18:04 PDT, Jan Erik Hanssen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Grace Kloba 2009-05-25 19:58:37 PDT
HTMLKeygenElement is derived from HTMLKeygenElement. But HTMLOptionElement::ownerSelectElement() only checks the selectTag.
Comment 1 Grace Kloba 2009-05-25 20:01:03 PDT
Sorry I meant HTMLKeygenElement is derived from HTMLSelectElement.
Comment 2 Grace Kloba 2009-05-25 20:21:22 PDT
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.
Comment 3 Darin Adler 2009-05-26 07:31:52 PDT
(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 4 Darin Adler 2009-05-26 07:36:00 PDT
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.
Comment 5 Grace Kloba 2009-05-26 17:20:04 PDT
Created attachment 30688 [details]
layout test file

Here is the layout test file. Which directory do you suggest me to put it?
Comment 6 Darin Adler 2009-05-26 17:21:30 PDT
(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.
Comment 7 Grace Kloba 2009-05-26 18:05:34 PDT
Created attachment 30689 [details]
new patch file with layout test
Comment 8 Grace Kloba 2009-05-26 18:23:05 PDT
Created attachment 30690 [details]
remove a tab from the previous layout file
Comment 9 Grace Kloba 2009-05-26 20:06:13 PDT
Created attachment 30693 [details]
remove another tab from the Changelog
Comment 10 Nikolas Zimmermann 2009-05-28 17:19:03 PDT
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?
Comment 11 Adam Barth 2009-06-02 01:09:02 PDT
Will land.
Comment 12 Adam Barth 2009-06-02 01:38:12 PDT
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 13 Adam Barth 2009-06-02 02:13:42 PDT
Comment on attachment 30693 [details]
remove another tab from the Changelog

The enclosed patch does not pass.
Comment 14 Adam Barth 2009-06-02 02:13:56 PDT
Reverting...
Comment 15 Grace Kloba 2009-06-15 10:52:14 PDT
What part doesn't the patch pass?
Comment 16 Adam Barth 2010-03-31 10:58:28 PDT
Comment on attachment 30693 [details]
remove another tab from the Changelog

Re-nominate for review to run through the EWS.
Comment 17 Adam Barth 2010-03-31 10:59:45 PDT
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 18 Adam Barth 2010-04-01 00:13:12 PDT
Comment on attachment 30693 [details]
remove another tab from the Changelog

Still looks good.  Forwarding Darin Adler's r+.
Comment 19 WebKit Commit Bot 2010-04-01 01:41:40 PDT
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 20 Adam Barth 2010-04-01 09:16:01 PDT
Comment on attachment 30693 [details]
remove another tab from the Changelog

Looks like the problem was that the test doesn't pass.
Comment 21 Jan Erik Hanssen 2010-04-15 17:21:10 PDT
Created attachment 53493 [details]
Proposed patch: Fix the test case
Comment 22 WebKit Review Bot 2010-04-15 17:24:37 PDT
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.
Comment 23 Jan Erik Hanssen 2010-04-15 18:04:09 PDT
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
Comment 24 Grace Kloba 2010-04-28 04:33:04 PDT
Ping? The patch had an r+ originally, the current patch just fixes a style error in the test file.

Thanks!
Comment 25 Adam Barth 2010-04-28 15:06:51 PDT
Comment on attachment 53496 [details]
Proposed patch: Fix the test case and the style error of the last patch

Forwarding r+.  Thanks!
Comment 26 WebKit Commit Bot 2010-04-29 02:33:51 PDT
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>
Comment 27 WebKit Commit Bot 2010-04-29 02:33:58 PDT
All reviewed patches have been landed.  Closing bug.