[Webkit-unassigned] [Bug 137553] abandoned select option is reselected when shift selecting new options

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=137553

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #239977|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

--- Comment #20 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 239977
  --> https://bugs.webkit.org/attachment.cgi?id=239977
After review

View in context: https://bugs.webkit.org/attachment.cgi?id=239977&action=review

r- because there are still some issues in the test and my previous comment wasn't addressed in the new patch.

> LayoutTests/fast/forms/listbox-selection-after-typeahead-expected.txt:8
> +mouseDownOnSelect(1)
> +PASS selectionPattern() is "0100000"

We should put these lines into the same line as in:
mouseDownOnSelect(1); selectionPattern()

> LayoutTests/fast/forms/listbox-selection-after-typeahead-expected.txt:10
> +keyDownOnSelect("M")
> +PASS selectionPattern() is "0000010"

It's not obvious to me why selecting "M" would result in 5th element being selected.
Why don't we rename the value so that we can call keyDownOnSelect("5")?

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:10
> +    select {
> +        border: none;
> +        margin: 0px;
> +        padding: 0px;
> +    }

What's the point of this style? Is this required?
Can't we specify in the inline style attribute?

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:26
> +function mouseDownOnSelect(index)

I don't think this function name makes sense.
What we're doing here is selecting an option by a mouse down.
I would call this mouseDownAtOption(index)

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:35
> +function keyDownOnSelect(identifier, modifier) {

Again, I don't think keyDownOnSelect is a descriptive name for this function.
How about sendKeyDownAfterFocusingSelect?
But do we really need to focus select element each time?
It seems like the focus should be on the select element after the first test case.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:41
> +function selectionPattern()

Once again, I don't understand what selectionPattern means.
Please use a more descriptive function name such as bitPatternForSelectedOptions.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:51
> +    + '<a href="https://bugs.webkit.org/show_bug.cgi?id=137553">https://bugs.webkit.org/show_bug.cgi?id=137553</a></br>'
> +    + '<i>Abandoned select option is reselected when shift selecting new options</i>.</p>');

Like Chris pointed out, we shouldn't be linkifying the bug inside here.
It's almost never useful to mention that bug this test is for since that's recoded in the change log
which is available through svn commit log and on trac.webkit.org.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:53
> +// Select 2nd option

Please remove this comment. This is "what" comment we avoid in WebKit. Only add "why" comment.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:55
> +evalAndLog('mouseDownOnSelect(1)');
> +shouldBeEqualToString('selectionPattern()', "0100000");

Combine these two lines as in:
shouldBeEqualToString('mouseDownOnSelect(1); selectionPattern()', "0100000");

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:57
> +// Typeahead to select the "Maroon" options

Ditto about removing the comment.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:59
> +evalAndLog('keyDownOnSelect("M")');
> +shouldBeEqualToString('selectionPattern()', "0000010");

Ditto about merging these two lines.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:61
> +// Start a multiple selection

Ditto about removing the comment.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:63
> +evalAndLog('keyDownOnSelect("upArrow", "shiftKey")');
> +shouldBeEqualToString('selectionPattern()', "0000110");

Ditto about merging these two lines.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141018/6e909cf7/attachment-0002.html>