Bug 71128 - Select multiple options with mouse drag in Select element.
Summary: Select multiple options with mouse drag in Select element.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-28 11:56 PDT by Rakesh
Modified: 2011-11-09 01:08 PST (History)
6 users (show)

See Also:


Attachments
Test case (687 bytes, text/html)
2011-10-28 11:56 PDT, Rakesh
no flags Details
Proposed patch (5.64 KB, patch)
2011-10-28 12:25 PDT, Rakesh
no flags Details | Formatted Diff | Diff
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. (5.81 KB, patch)
2011-11-02 11:07 PDT, Rakesh
no flags Details | Formatted Diff | Diff
Updated patch (6.02 KB, patch)
2011-11-08 11:15 PST, Rakesh
no flags Details | Formatted Diff | Diff
Updated patch (5.92 KB, patch)
2011-11-08 12:38 PST, Rakesh
no flags Details | Formatted Diff | Diff
Updated patch (5.85 KB, patch)
2011-11-08 22:54 PST, Rakesh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rakesh 2011-10-28 11:56:58 PDT
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.
Comment 1 Rakesh 2011-10-28 12:25:38 PDT
Created attachment 112892 [details]
Proposed patch
Comment 2 Rakesh 2011-10-28 12:29:54 PDT
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.
Comment 3 Alexey Proskuryakov 2011-10-28 13:11:41 PDT
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.
Comment 4 Rakesh 2011-10-28 21:26:36 PDT
(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.
Comment 5 Alexey Proskuryakov 2011-10-28 23:53:39 PDT
>> 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.
Comment 6 Rakesh 2011-10-31 00:23:48 PDT
(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.
Comment 7 Ryosuke Niwa 2011-10-31 00:28:32 PDT
(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.
Comment 8 Ryosuke Niwa 2011-10-31 00:32:26 PDT
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.
Comment 9 Rakesh 2011-10-31 03:07:35 PDT
(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.
Comment 10 Ryosuke Niwa 2011-10-31 06:38:00 PDT
(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?
Comment 11 Rakesh 2011-10-31 09:10:55 PDT
(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.
Comment 12 Ryosuke Niwa 2011-10-31 09:39:05 PDT
(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.
Comment 13 Rakesh 2011-10-31 11:57:29 PDT
(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?
Comment 14 Ryosuke Niwa 2011-10-31 11:58:03 PDT
(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.
Comment 15 Kent Tamura 2011-11-02 01:22:06 PDT
(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.
Comment 16 Kent Tamura 2011-11-02 01:27:41 PDT
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 '+'
Comment 17 Rakesh 2011-11-02 11:07:50 PDT
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.
Comment 18 Ryosuke Niwa 2011-11-07 23:09:08 PST
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.
Comment 19 Rakesh 2011-11-08 03:00:32 PST
(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?
Comment 20 Ryosuke Niwa 2011-11-08 08:28:48 PST
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?
Comment 21 Ryosuke Niwa 2011-11-08 09:41:24 PST
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.
Comment 22 Rakesh 2011-11-08 11:15:59 PST
Created attachment 114121 [details]
Updated patch

Removed duplication of the condition
Comment 23 Ryosuke Niwa 2011-11-08 11:22:48 PST
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.
Comment 24 Rakesh 2011-11-08 11:30:31 PST
(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
Comment 25 Ryosuke Niwa 2011-11-08 11:50:06 PST
(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?
Comment 26 Rakesh 2011-11-08 12:38:42 PST
Created attachment 114136 [details]
Updated patch

Removed multiple function calls for renderer
Comment 27 Ryosuke Niwa 2011-11-08 15:26:29 PST
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()
Comment 28 Rakesh 2011-11-08 22:54:59 PST
Created attachment 114210 [details]
Updated patch

Changes as per Comment #27
Comment 29 WebKit Review Bot 2011-11-09 00:56:51 PST
Comment on attachment 114210 [details]
Updated patch

Clearing flags on attachment: 114210

Committed r99667: <http://trac.webkit.org/changeset/99667>
Comment 30 WebKit Review Bot 2011-11-09 00:56:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 31 Rakesh 2011-11-09 01:08:32 PST
(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+.