| 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: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Mac | ||||||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Grace Kloba
2009-05-25 19:58:37 PDT
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. |