Bug 71128 – Select multiple options with mouse drag in Select element.

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

[Rakesh]

Created attachment 112889 [details]
Test case

Selection of multiple elements does not happen with mouse drag in select element.

Steps to reproduce:
1. Focus on select element.
2. Drag on the select element.
3. It should select options from the field.

Behavior observed:

Selection happens sometimes when mouse sometimes when the mouse is moved randomly on the select element.

Expected:
Selection should happen smoothly when dragged on select element.

[Rakesh]

Created attachment 112892 [details]
Proposed patch

[Rakesh]

As the optional element in Select does not have a renderer, when the drag happens the node under mouse is option it returns without firing auto scroll. With the change in proposed patch, we check if the parent has renderer and if it can autoscroll then allow autoscroll to happen.

[Alexey Proskuryakov]

What is the difference with bug 70496?

You didn't provide a rationale for this change. I see that Finder has remotely similar behavior, but I'm not sure if this can be considered platform convention for Mac or other platforms.

[Rakesh]

(In reply to comment #3)
> What is the difference with bug 70496?
> 
This change will not have effect on this 70496 as "canAutoscroll" fails for the select element with size more than the option elements which is the case for "Priority" in the link given for bug. (Sorry, the description of that issue is not clear and will update it accordingly)

> You didn't provide a rationale for this change. 
I explained the change in #c2.

>I see that Finder has remotely similar behavior, but I'm not sure if this can be considered platform convention for Mac or other platforms.

I have not used mac, so not aware of 'Finder' behavior(will try to see if can get hold of one) but it makes selection easy with just mouse usage.

[Alexey Proskuryakov]

>> You didn't provide a rationale for this change. 
> I explained the change in #c2.

That's not what "rationale" means. You need to explain why you are making this change, not what the change is.

We can't be doing everything that just seems like a good idea. Web browsers run alongside other applications, and should provide consistent user experience.

[Rakesh]

(In reply to comment #5)

> That's not what "rationale" means. You need to explain why you are making this change, not what the change is.

Sorry to get that wrong, both firefox and opera as well as IE support this feature.

> 
> We can't be doing everything that just seems like a good idea. Web browsers run alongside other applications, and should provide consistent user experience.

Yes, agreed but other browsers do support this feature.

[Ryosuke Niwa]

(In reply to comment #6)
> (In reply to comment #5)
> > That's not what "rationale" means. You need to explain why you are making this change, not what the change is.
> 
> Sorry to get that wrong, both firefox and opera as well as IE support this feature.

Firefox and IE usually follow Windows conventions so we have to be careful to conclude that's the right behavior.

> > We can't be doing everything that just seems like a good idea. Web browsers run alongside other applications, and should provide consistent user experience.
> 
> Yes, agreed but other browsers do support this feature.

Having said that this behavior seems fine. XCode's recent projects list, for example, allows multiple items to be selected by a mouse drag.

[Ryosuke Niwa]

Comment on attachment 112892 [details]
Proposed patch

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

> Source/WebCore/ChangeLog:15
> +        Allow auto scroll to be fired if current node under mouse does not have renderer
> +        but its container node can autoscroll and has renderer.

Please explain how this fixes the bug.

[Rakesh]

(In reply to comment #8)
> (From update of attachment 112892 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112892&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        Allow auto scroll to be fired if current node under mouse does not have renderer
> > +        but its container node can autoscroll and has renderer.
> 
> Please explain how this fixes the bug.

Presently we return if don't find a renderer for targetNode in EventHandler::handleMouseDraggedEvent() :

if (event.event().button() != LeftButton || !targetNode || !targetNode->renderer())

When we do a drag on the option element we don't get a renderer but as its parent(select) element has a renderer and it can scroll, we can allow auto scroll for parent.

In short, as we want to autoscroll on any renderer that is autoscrollable in its tree, allow it even when present node does not have renderer but its parent has.

[Ryosuke Niwa]

(In reply to comment #9)
> Presently we return if don't find a renderer for targetNode in EventHandler::handleMouseDraggedEvent() :
> 
> if (event.event().button() != LeftButton || !targetNode || !targetNode->renderer())
> 
> When we do a drag on the option element we don't get a renderer but as its parent(select) element has a renderer and it can scroll, we can allow auto scroll for parent.

Then we must be detecting this specific case, not always fallback to the parent node.

> In short, as we want to autoscroll on any renderer that is autoscrollable in its tree, allow it even when present node does not have renderer but its parent has.

That seems wrong. On one hand, we need a justification as to why it's okay to fallback to its parent. In the case of option and select, it's clear but I'm not convinced that it's always okay for other cases.

On the other hand, it's also unclear to me why we only need to care about the parent node if we're applying this to any node. Why, for example, don't we go up the tree until we find an ancestor with renderer?

[Rakesh]

(In reply to comment #10)
> (In reply to comment #9)
> > Presently we return if don't find a renderer for targetNode in EventHandler::handleMouseDraggedEvent() :
> > 
> > if (event.event().button() != LeftButton || !targetNode || !targetNode->renderer())
> > 
> > When we do a drag on the option element we don't get a renderer but as its parent(select) element has a renderer and it can scroll, we can allow auto scroll for parent.
> 
> Then we must be detecting this specific case, not always fallback to the parent node.
> 
> > In short, as we want to autoscroll on any renderer that is autoscrollable in its tree, allow it even when present node does not have renderer but its parent has.
> 
> That seems wrong. On one hand, we need a justification as to why it's okay to fallback to its parent. In the case of option and select, it's clear but I'm not convinced that it's always okay for other cases.

I thought of textarea as example where text scrolls within textarea or text within an scrollable div.

> 
> On the other hand, it's also unclear to me why we only need to care about the parent node if we're applying this to any node. Why, for example, don't we go up the tree until we find an ancestor with renderer?

Finding an ancestor with renderer looks to me a good option as we are finding a renderer which can auto-scroll from present node. 

Please let me know if its fine, I will upload a patch for the same.

[Ryosuke Niwa]

(In reply to comment #11)
> (In reply to comment #10)
> > That seems wrong. On one hand, we need a justification as to why it's okay to fallback to its parent. In the case of option and select, it's clear but I'm not convinced that it's always okay for other cases.
> 
> I thought of textarea as example where text scrolls within textarea or text within an scrollable div.

In such cases, the anchor node should be text and have a renderer.

> > On the other hand, it's also unclear to me why we only need to care about the parent node if we're applying this to any node. Why, for example, don't we go up the tree until we find an ancestor with renderer?
> 
> Finding an ancestor with renderer looks to me a good option as we are finding a renderer which can auto-scroll from present node. 

Don't think so.

[Rakesh]

(In reply to comment #12)

> In such cases, the anchor node should be text and have a renderer.

Sorry, I didn't get this, did you mean option node?

[Ryosuke Niwa]

(In reply to comment #13)
> (In reply to comment #12)
> 
> > In such cases, the anchor node should be text and have a renderer.
> 
> Sorry, I didn't get this, did you mean option node?

No in textarea.

[Kent Tamura]

(In reply to comment #4)
> (In reply to comment #3)
> > What is the difference with bug 70496?
> > 
> This change will not have effect on this 70496 as "canAutoscroll" fails for the select element with size more than the option elements which is the case for "Priority" in the link given for bug. (Sorry, the description of that issue is not clear and will update it accordingly)

I'm  still confused with this patch and Bug 70496. The test in your patch looks to confirm Bug 70496 is fixed. Please clarify the description of this bug and the patch.

(In reply to comment #10)
> (In reply to comment #9)
> > Presently we return if don't find a renderer for targetNode in EventHandler::handleMouseDraggedEvent() :
> > 
> > if (event.event().button() != LeftButton || !targetNode || !targetNode->renderer())
> > 
> > When we do a drag on the option element we don't get a renderer but as its parent(select) element has a renderer and it can scroll, we can allow auto scroll for parent.
> 
> Then we must be detecting this specific case, not always fallback to the parent node.

I agree with Ryosuke.  We should have isListBox() check.

[Kent Tamura]

Comment on attachment 112892 [details]
Proposed patch

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

> Source/WebCore/ChangeLog:8
> +        Selection in select element with an mouse drag.

Please add rationale to ChangeLog.

> LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag.html:5
> +<link rel="stylesheet" href="../js/resources/js-test-style.css">

This line is not needed. js-test-pre.js automatically loads it.

> LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag.html:11
> +    if(window.eventSender) {

Though we don't have an official code style for JavaScript, please follow the C++ style as possible.
Need a space after 'if'

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

Need a space after ','

> LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag.html:21
> +        eventSender.mouseMoveTo(x,y + (optionHeight * 3));

Need a space after ','

> LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag.html:33
> +    for(var i=0; i < 4; i++) {

Need a space
 after 'for'
 before and after '='

> LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag.html:34
> +        shouldBeTrue("document.getElementById(\"selectId\").options["+ i + "].selected");

Need a space before the first '+'

[Rakesh]

Created attachment 113337 [details]
Handling the specific case of allowing the autoscroll for parent node when the current node under mouse does not have renderer in case of select(parent) and option element.

[Ryosuke Niwa]

Comment on attachment 113337 [details]
Handling the specific case of allowing the autoscroll for parent node when the current node under mouse does not have renderer in case of select(parent) and option element.

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

> Source/WebCore/page/EventHandler.cpp:561
> +          || !(targetNode->renderer() || (targetNode->parentNode()->renderer() && targetNode->parentNode()->renderer()->isListBox())))

We should probably define a boolean here.

> Source/WebCore/page/EventHandler.cpp:573
> +        if (!renderer && targetNode->parentNode()->renderer()->isListBox())

To avoid repeating the condition here.

> LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag.html:9
> +window.jsTestIsAsync = true;
> +
> +function test() {

This test doesn't need to be async. r- because of this.

> LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag.html:20
> +        eventSender.mouseDown();
> +        eventSender.mouseMoveTo(x, y + (optionHeight * 3));

You probably need leapForward here.

[Rakesh]

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

Thanks for your inputs, will make those changes.

> > LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag.html:9
> > +window.jsTestIsAsync = true;
> > +
> > +function test() {
> 
> This test doesn't need to be async. r- because of this.
> 

As autoscroll happens on timer, I thought test may need to be async.

> > LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag.html:20
> > +        eventSender.mouseDown();
> > +        eventSender.mouseMoveTo(x, y + (optionHeight * 3));
> 
> You probably need leapForward here.

As suggested, I tried using leapForward without setTimeout but it does not give required output. Please let me know if I am doing something wrong or we need to have async test if it involves timers?

[Ryosuke Niwa]

Comment on attachment 113337 [details]
Handling the specific case of allowing the autoscroll for parent node when the current node under mouse does not have renderer in case of select(parent) and option element.

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

> LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag.html:22
> +        setTimeout(testSelection, 100);

Why do you need to use setTimeout?

[Ryosuke Niwa]

Okay, apparently the autoscroll happens on timer so keeping the test async is fine. But please define a boolean variable in EventHandler.cpp to avoid duplicating the condition.

[Rakesh]

Created attachment 114121 [details]
Updated patch

Removed duplication of the condition

[Ryosuke Niwa]

Comment on attachment 114121 [details]
Updated patch

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

> Source/WebCore/page/EventHandler.cpp:567
> +        if (!(targetNode->parentNode() && targetNode->parentNode()->renderer() && targetNode->parentNode()->renderer()->isListBox()))
> +            return false;
> +        isListBox = true;

Hm... on my second thought it's probably cleaner to keep the pointer to targetNode->parentNode()->renderer() because you're calling it again in line 580.

[Rakesh]

(In reply to comment #23)
> (From update of attachment 114121 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114121&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:567
> > +        if (!(targetNode->parentNode() && targetNode->parentNode()->renderer() && targetNode->parentNode()->renderer()->isListBox()))
> > +            return false;
> > +        isListBox = true;
> 
> Hm... on my second thought it's probably cleaner to keep the pointer to targetNode->parentNode()->renderer() because you're calling it again in line 580.

Ya, can we bring the RenderObject* renderer = targetNode->renderer(); up so that we can avoid even targetNode->renderer() twice but only thing we may be bringing out a if scoped variable? That will even avoid an extra local variable

[Ryosuke Niwa]

(In reply to comment #24)
> Ya, can we bring the RenderObject* renderer = targetNode->renderer(); up so that we can avoid even targetNode->renderer() twice but only thing we may be bringing out a if scoped variable? That will even avoid an extra local variable

I don't really follow... Can we talk about this on IRC?

[Rakesh]

Created attachment 114136 [details]
Updated patch

Removed multiple function calls for renderer

[Ryosuke Niwa]

Comment on attachment 114136 [details]
Updated patch

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

> Source/WebCore/page/EventHandler.cpp:566
> +        if (!(renderer && renderer->isListBox()))

It's probably simpler to write this as !renderer || !renderer->isListBox()

[Rakesh]

Created attachment 114210 [details]
Updated patch

Changes as per Comment #27

[WebKit Review Bot]

Comment on attachment 114210 [details]
Updated patch

Clearing flags on attachment: 114210

Committed r99667: <http://trac.webkit.org/changeset/99667>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Rakesh]

(In reply to comment #29)
> (From update of attachment 114210 [details])
> Clearing flags on attachment: 114210
> 
> Committed r99667: <http://trac.webkit.org/changeset/99667>
Thanks for reviewing and r+ and commit+.