RESOLVED FIXED 35786
Improve portability of fast/forms/listbox-selection.html
https://bugs.webkit.org/show_bug.cgi?id=35786
Summary Improve portability of fast/forms/listbox-selection.html
Kent Tamura
Reported 2010-03-05 02:33:03 PST
listbox-selection.html has potability problems: - it depends on Mac-specific metrics (itemHeight = 14) - it changes behavior by navigator.userAgent string We should detect the item height at runtime like Bug#34072. We should use "addSelectionKey" modifier for metaKey (Mac) or controlKey (others) like Bug#33989.
Attachments
Patch (18.35 KB, patch)
2010-08-05 22:40 PDT, Kenichi Ishibashi
no flags
Patch V1 (19.24 KB, patch)
2010-08-18 01:20 PDT, Kenichi Ishibashi
no flags
Kenichi Ishibashi
Comment 1 2010-08-05 22:40:28 PDT
Kent Tamura
Comment 2 2010-08-06 00:37:47 PDT
Comment on attachment 63693 [details] Patch The change is basically good. Thank you for taking this bug. Did you confirm the change worked on Mac and Windows? > + Improve portability of fast/forms/listbox-selection.html > + https://bugs.webkit.org/show_bug.cgi?id=35786 > + > + * fast/forms/listbox-selection-expected.txt: Updated to match with new test. > + * fast/forms/listbox-selection.html: Modified to use script-tests style. > + * fast/forms/script-tests/listbox-selection.js: Added. > + (mouseDownOnSelect): > + (keyDownOnSelect): > + (createSelect): > + (selectionPattern): You had better add how to resolve the portability issue to ChangeLog. > --- a/LayoutTests/fast/forms/listbox-selection-expected.txt > +++ b/LayoutTests/fast/forms/listbox-selection-expected.txt > @@ -1,29 +1,23 @@ > - 1) Select one item with mouse (no previous selection) > - 2) Select one item with mouse (with previous selection) > - 3) Select one item with the keyboard (no previous selection) > - 4) Select one item with the keyboard (with previous selection) > - 5) Attempt to select an item cmd-clicking > - 6) Attempt to select a range shift-clicking > - 7) Attempt to select a range with the keyboard > - 8) Select one item with mouse (no previous selection) > - 9) Select one item with mouse (with previous selection) > - 10) Select one item with the keyboard (no previous selection) > - 11) Select one item with the keyboard (with previous selection) > - 12) Select an item cmd-clicking > - 13) Select a range shift-clicking > - 14) Select a range with the keyboard The old expectation had information about what was tested, and new expectation has no such information. An expectation files should have such information in order to detect what is wrong easily when a test fails. So, for example, > +// 1) Select one item with mouse (no previous selection) > +mouseDownOnSelect("sl1", 0); > +shouldBe('selectionPattern("sl1")', '"10000"'); The first line should be: debug("1) Select one item with mouse (no previous selection)") > +if (window.layoutTestController) > + layoutTestController.waitUntilDone(); > +if (window.layoutTestController) > + layoutTestController.notifyDone(); You don't need these blocks because js-test-pre.js and js-test-post.js are loaded.
Kenichi Ishibashi
Comment 3 2010-08-06 01:23:10 PDT
Kent-san, Thank you for your quick review! I really appreciate your kind suggestions. Your comments definitely help me because I am new to WebKit development. Unfortunately, I don't have a windows machine that can be used to test for now, so I'll send revised patch after I get a windows machine. Thanks,
Kenichi Ishibashi
Comment 4 2010-08-18 01:16:37 PDT
I've finally prepared a windows machine for the test and the test is successfully passed on the windows machine. I'm going to send a review request for revised patch soon. Thanks,
Kenichi Ishibashi
Comment 5 2010-08-18 01:20:51 PDT
Created attachment 64674 [details] Patch V1
Kent Tamura
Comment 6 2010-08-19 19:25:09 PDT
Comment on attachment 64674 [details] Patch V1 OK.
WebKit Commit Bot
Comment 7 2010-08-19 20:37:52 PDT
Comment on attachment 64674 [details] Patch V1 Clearing flags on attachment: 64674 Committed r65723: <http://trac.webkit.org/changeset/65723>
WebKit Commit Bot
Comment 8 2010-08-19 20:37: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.