Bug 35786 – Improve portability of fast/forms/listbox-selection.html

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kent Tamura]

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.

[Kenichi Ishibashi]

Created attachment 63693 [details]
Patch

[Kent Tamura]

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]

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]

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]

Created attachment 64674 [details]
Patch V1

[Kent Tamura]

Comment on attachment 64674 [details]
Patch V1

OK.

[WebKit Commit Bot]

Comment on attachment 64674 [details]
Patch V1

Clearing flags on attachment: 64674

Committed r65723: <http://trac.webkit.org/changeset/65723>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.