Bug 70496 – Cannot select multiple options by mouse dragging in <select multiple="multiple" size="7"> list

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Rakesh]

In option lists created by <select multiple="multiple" size="7">,Cannot select multiple items at once with the mouse by dragging over a range (i.e. press mouse down on first item, move to last item, release mouse). In other browsers (FF 4, IE 8), this works as expected


Chromium Issue: http://code.google.com/p/chromium/issues/detail?id=85067

[Rakesh]

(In reply to comment #0)
> In option lists created by <select multiple="multiple" size="7">,Cannot select multiple items at once with the mouse by dragging over a range (i.e. press mouse down on first item, move to last item, release mouse). In other browsers (FF 4, IE 8), this works as expected
> 
> 
> Chromium Issue: http://code.google.com/p/chromium/issues/detail?id=85067

This issue happens only for "Priority" select in the link. For all others where the number of options is more than size of the select element, https://bugs.webkit.org/show_bug.cgi?id=71128 has been logged.

[Rakesh]

Created attachment 114266 [details]
Proposed patch

Issue 71128 fixes the selection of option elements with an mouse drag when no. of options are more than the size attribute.
To handle the selection when no. of options are less than size attribute, we handle the mouse move event and select the options when mouse moves on the option element.
We also dont want to do this selection if the select element can be autoscrolled(i.e no. of options > size)

[Kent Tamura]

Comment on attachment 114266 [details]
Proposed patch

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

> Source/WebCore/html/HTMLSelectElement.cpp:1208
> +    } else if (event->type() == eventNames().mousemoveEvent && event->isMouseEvent()
> +                  && static_cast<MouseEvent*>(event)->button() == LeftButton && static_cast<MouseEvent*>(event)->buttonDown()
> +                  && !static_cast<MouseEvent*>(event)->ctrlKey() && !toRenderBox(renderer())->canBeScrolledAndHasScrollableArea()) {

* The indentation looks wrong for folded lines. You don't need to fold a long lines.

* You don't need to check these conditions in one 'if' statement. You can write:

} else if (event->type() == evnetNames().mousemoveEvent && event->isMouseEvent()) {
    MouseEvent* mouseEvent = static_cast<MouseEvent*>(event);
    If (mouseEvent->button() != LeftButton || !mouseEvent->buttonDown())
        return;
    ....

* Checking only ctrlKey() is wrong. It should be metaKey() in Mac.  Please refeer to mousedown handling in this function.

* Do we need to check !canBeScrolledAndHasScrollableArea()?

> Source/WebCore/html/HTMLSelectElement.cpp:1209
> +        // Convert to coords relative to the list box if needed.

This comment is not helpful.  Please remove it.

[Rakesh]

Created attachment 114447 [details]
Updated patch

[Rakesh]

(In reply to comment #3)
> (From update of attachment 114266 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114266&action=review

Thanks for reviewing the patch.
> 
> > Source/WebCore/html/HTMLSelectElement.cpp:1208
> > +    } else if (event->type() == eventNames().mousemoveEvent && event->isMouseEvent()
> > +                  && static_cast<MouseEvent*>(event)->button() == LeftButton && static_cast<MouseEvent*>(event)->buttonDown()
> > +                  && !static_cast<MouseEvent*>(event)->ctrlKey() && !toRenderBox(renderer())->canBeScrolledAndHasScrollableArea()) {
> 
> * The indentation looks wrong for folded lines. You don't need to fold a long lines.
> 
> * You don't need to check these conditions in one 'if' statement. You can write:
> 
> } else if (event->type() == evnetNames().mousemoveEvent && event->isMouseEvent()) {
>     MouseEvent* mouseEvent = static_cast<MouseEvent*>(event);
>     If (mouseEvent->button() != LeftButton || !mouseEvent->buttonDown())
>         return;
>     ....
> 

Made these changes.

> * Checking only ctrlKey() is wrong. It should be metaKey() in Mac.  Please refeer to mousedown handling in this function.
> 

Yes, now I have removed the ctrl/meta key case to match the selection behaviour similar to 71128 case instead of rejecting the selection if ctrl/meta is pressed.

> * Do we need to check !canBeScrolledAndHasScrollableArea()?
> 
Without this check autoscroll will not happen as mouse move event get handled here.

[Kent Tamura]

Comment on attachment 114447 [details]
Updated patch

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

> Source/WebCore/ChangeLog:12
> +        Test: fast/forms/select-multiple-elements-with-mouse-drag-with-options-less-than-size.html

Test coverage is not good.
We should test:
 - Dragging upward in a multiple listbox
 - Dragging in a multiple listbox with "addSelectionKey"
 - Dragging in a multiple listbox with "rangeSelectionKey"
 - Dragging in a non-multiple listbox
 - Dragging in a non-multiple listbox with "addSelectionKey"
 - Dragging in a non-multiple listbox with "rangeSelectionKey"

> Source/WebCore/html/HTMLSelectElement.cpp:1218
> +                updateSelectedState(listIndex, mouseEvent->ctrlKey(), mouseEvent->shiftKey());

I found passing ctrlKey() and shiftKey() is meaningless here because they are required only if m_multiple.

[Rakesh]

Created attachment 114672 [details]
Updated patch

Changes as per Comment #6. Passing false instead of  ctrlKey() and shiftKey().

[Kent Tamura]

Comment on attachment 114672 [details]
Updated patch

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

> LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag-with-options-less-than-size.html:30
> +<script>
> +    if (window.eventSender) {

You don't need to indent the whole content of <script>.

> LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag-with-options-less-than-size.html:50
> +        eventSender.mouseMoveTo(x,y);

should have a space after ","

> LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag-with-options-less-than-size.html:59
> +        eventSender.mouseDown();
> +        eventSender.mouseDown(0, "addSelectionKey");
> +        eventSender.mouseMoveTo(x, y + (optionHeight * 3));
> +        eventSender.mouseUp(0, "addSelectionKey");
> +        testOptionSelection(0, 4, "true", "selectId");

This test is meaningless. We should test a case where
 - an option is selected.  e.g. options[0] is selected
 - start dragging with addSelectionKey at another option. e.g. starting at options[2]
 - end dragging with addSelectionKey at yet another option.  e.g. ending at options[4]
 - then check if the selection is correctly added; options[0], [2] [3] [4] should be selected.

> LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag-with-options-less-than-size.html:64
> +        eventSender.mouseMoveTo(x,y);

should have a space after ","

> LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag-with-options-less-than-size.html:87
> +        eventSender.mouseMoveTo(x,y);

ditto.

> LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag-with-options-less-than-size.html:103
> +        eventSender.mouseMoveTo(x,y);

ditto.

[Rakesh]

Created attachment 114891 [details]
Updated patch

Updated test case as suggested in Comment #8.

[Kent Tamura]

Comment on attachment 114891 [details]
Updated patch

Looks ok.  Thank you for fixing this!

[WebKit Review Bot]

Comment on attachment 114891 [details]
Updated patch

Clearing flags on attachment: 114891

Committed r100111: <http://trac.webkit.org/changeset/100111>

[Ryosuke Niwa]

Test added by this patch is failing on Mac:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r101094%20(34915)/fast/forms/select-multiple-elements-with-mouse-drag-with-options-less-than-size-pretty-diff.html

[Ryosuke Niwa]

Fixed the test in http://trac.webkit.org/changeset/102215 since it had been failing on Mac.