Bug 26016

Summary:

HTMLOptionElement::ownerSelectElement() needs to consider keygen

Product:

WebKit

Reporter:

Grace Kloba <klobag>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, android-webkit-unforking, commit-queue, jchaffraix, jhanssen, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

Attachments:

Description	Flags
fix bug	darin: review-
layout test file	none
new patch file with layout test	none
remove a tab from the previous layout file	none
remove another tab from the Changelog	abarth: review-, commit-queue: commit-queue-
Proposed patch: Fix the test case	none
Proposed patch: Fix the test case and the style error of the last patch	none

Grace Kloba

Reported 2009-05-25 19:58:37 PDT

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

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
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Grace Kloba

Comment 1 2009-05-25 20:01:03 PDT

Sorry I meant HTMLKeygenElement is derived from HTMLSelectElement.

Grace Kloba

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

Darin Adler

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

Darin Adler

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

Grace Kloba

Comment 5 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?

Darin Adler

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

Grace Kloba

Comment 7 2009-05-26 18:05:34 PDT

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

Grace Kloba

Comment 8 2009-05-26 18:23:05 PDT

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

Grace Kloba

Comment 9 2009-05-26 20:06:13 PDT

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

Nikolas Zimmermann

Comment 10 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?

Adam Barth

Comment 11 2009-06-02 01:09:02 PDT

Will land.

Adam Barth

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

Adam Barth

Comment 13 2009-06-02 02:13:42 PDT

Comment on attachment 30693 [details] remove another tab from the Changelog The enclosed patch does not pass.

Adam Barth

Comment 14 2009-06-02 02:13:56 PDT

Reverting...

Grace Kloba

Comment 15 2009-06-15 10:52:14 PDT

What part doesn't the patch pass?

Adam Barth

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

Adam Barth

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

Adam Barth

Comment 18 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+.

WebKit Commit Bot

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

Adam Barth

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

Jan Erik Hanssen

Comment 21 2010-04-15 17:21:10 PDT

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

WebKit Review Bot

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

Jan Erik Hanssen

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

Grace Kloba

Comment 24 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!

Adam Barth

Comment 25 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!

WebKit Commit Bot

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

WebKit Commit Bot

Comment 27 2010-04-29 02:33:58 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.