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.
Created attachment 63693 [details] Patch
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.
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,
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,
Created attachment 64674 [details] Patch V1
Comment on attachment 64674 [details] Patch V1 OK.
Comment on attachment 64674 [details] Patch V1 Clearing flags on attachment: 64674 Committed r65723: <http://trac.webkit.org/changeset/65723>
All reviewed patches have been landed. Closing bug.