Bug 39725 - Dragging over an element with scrollbars should scroll the element when dragging near edges
Summary: Dragging over an element with scrollbars should scroll the element when dragg...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: yosin
URL:
Keywords:
Depends on: 106318 41695 104764 106395 106702
Blocks: 106403
  Show dependency treegraph
 
Reported: 2010-05-26 04:23 PDT by Hajime Morrita
Modified: 2013-09-20 06:53 PDT (History)
22 users (show)

See Also:


Attachments
patch v0 (7.04 KB, patch)
2010-05-27 23:48 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v1 - fix style violation (7.05 KB, patch)
2010-05-27 23:55 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v2 (4.85 KB, patch)
2010-06-02 01:45 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v3 (4.97 KB, patch)
2010-06-02 18:12 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v4 (5.04 KB, patch)
2010-06-02 20:15 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Test case (847 bytes, text/html)
2010-06-21 12:10 PDT, Erik Arvidsson
no flags Details
patch v0 (38.76 KB, patch)
2010-07-07 02:17 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v1 fix style violation (38.69 KB, patch)
2010-07-07 02:53 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
sync to latest (38.58 KB, patch)
2010-09-08 03:50 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (38.50 KB, patch)
2010-10-25 01:15 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
WIP Patch 2.0 (20.71 KB, patch)
2012-11-30 03:05 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 2.0-1 (29.68 KB, patch)
2012-12-04 23:38 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 2.0-2 (30.43 KB, patch)
2012-12-05 23:19 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 3.0 (23.56 KB, patch)
2012-12-14 01:41 PST, yosin
no flags Details | Formatted Diff | Diff
Patch-3.0.1 (23.76 KB, patch)
2013-01-07 21:54 PST, yosin
no flags Details | Formatted Diff | Diff
Patch-3.0.2 (31.81 KB, patch)
2013-01-15 00:31 PST, yosin
no flags Details | Formatted Diff | Diff
Patch-3.0.3 (31.81 KB, patch)
2013-01-15 01:15 PST, yosin
no flags Details | Formatted Diff | Diff
Patch-3.0.4 (36.97 KB, patch)
2013-01-16 20:43 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 3.0.5 (36.84 KB, patch)
2013-01-18 01:33 PST, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2010-05-26 04:23:36 PDT
What steps will reproduce the problem?

1. Have an element with overflow auto/scroll which has enough content to 
require scrollbars to be shown
2. Drag an item (bookmark from the bookmark bar or a file from the desktop) 
near the edge of the element with the scrollbars.

What is the expected output?

The element should scroll so that the user gets access to things outside the 
viewport. The scrolling should be smooth.

What do you see instead?

On Windows this works when dragging near the top edge but not near the bottom 
edge. On Mac and Linux it does not work at all.

Chromium side is here: http://crbug.com/37320
Comment 1 Hajime Morrita 2010-05-27 23:48:35 PDT
Created attachment 57296 [details]
patch v0
Comment 2 WebKit Review Bot 2010-05-27 23:50:12 PDT
Attachment 57296 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebViewImpl.cpp:1670:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/chromium/src/WebViewImpl.cpp:1681:  Missing spaces around /  [whitespace/operators] [3]
WebKit/chromium/src/WebViewImpl.cpp:1690:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Hajime Morrita 2010-05-27 23:55:46 PDT
Created attachment 57298 [details]
patch v1 - fix style violation
Comment 4 Jian Li 2010-06-01 17:43:11 PDT
Comment on attachment 57298 [details]
patch v1 - fix style violation

WebCore/platform/graphics/IntRect.cpp:57
 +  IntSize IntRect::directionTo(const WebCore::IntPoint& target) const
The name directionTo seems to be confusing. Will it be better to distanceTo? Also better to comment what distance to rectangle is meant.
In addition, I am thinking if this routine is only needed in chromium platform, probably we do not want to add it to IntRect.*.

WebCore/platform/graphics/IntRect.h:119
 +      IntSize directionTo(const WebCore::IntPoint& target) const;
The argument name 'target' can be omitted.

WebKit/chromium/public/WebView.h:172
 +      // Notified the WebView that a dragging is going on.
Notified => Notifies

WebKit/chromium/public/WebView.h:173
 +      virtual void dragSourceMovedTo(
No need to fold into multiple lines.

WebKit/chromium/src/WebViewImpl.cpp:1550
 +  void WebViewImpl::dragSourceMovedTo(
ditto.

WebKit/chromium/src/WebViewImpl.cpp:1678
 +      WebCore::FrameView* frameView = mainFrameImpl()->frameView();
Do we need to check if frameView is NULL?

WebKit/chromium/src/WebViewImpl.cpp:1684
 +      bounds.setHeight(bounds.height() - margin*2);
Should have one space before and after '*'.

WebKit/chromium/src/WebViewImpl.cpp:1686
 +      bounds.setWidth(bounds.width() - margin*2);
ditto.

WebKit/chromium/src/WebViewImpl.h:136
 +      virtual void dragSourceMovedTo(
ditto.

WebKit/chromium/src/WebViewImpl.h:331
 +      static const int kScrollMarginRatio = 20;
This const definition should be moved to WebViewImpl::scrollForDragging. Also please comment why choosing value 20?

WebKit/chromium/src/WebViewImpl.h:353
 +      void scrollForDragging(const WebPoint& clientPoint);
The argument name can be omitted.
Comment 5 Hajime Morrita 2010-06-02 01:45:02 PDT
Created attachment 57634 [details]
patch v2
Comment 6 Hajime Morrita 2010-06-02 01:49:19 PDT
Hi Jian, thank you for reviewing!
I updated the patch.

(In reply to comment #4)
> (From update of attachment 57298 [details])
> WebCore/platform/graphics/IntRect.cpp:57
>  +  IntSize IntRect::directionTo(const WebCore::IntPoint& target) const
> The name directionTo seems to be confusing. Will it be better to distanceTo? Also better to comment what distance to rectangle is meant.
> In addition, I am thinking if this routine is only needed in chromium platform, probably we do not want to add it to IntRect.*.
Renamed, and moved it to WebViewImpl.cpp local

> 
> WebCore/platform/graphics/IntRect.h:119
>  +      IntSize directionTo(const WebCore::IntPoint& target) const;
> The argument name 'target' can be omitted.
Done.

> 
> WebKit/chromium/public/WebView.h:172
>  +      // Notified the WebView that a dragging is going on.
> Notified => Notifies
Done.
> 
> WebKit/chromium/public/WebView.h:173
>  +      virtual void dragSourceMovedTo(
> No need to fold into multiple lines.
Done.
> 
> WebKit/chromium/src/WebViewImpl.cpp:1550
>  +  void WebViewImpl::dragSourceMovedTo(
> ditto.
Done.

> 
> WebKit/chromium/src/WebViewImpl.cpp:1678
>  +      WebCore::FrameView* frameView = mainFrameImpl()->frameView();
> Do we need to check if frameView is NULL?
Guarded null case.

> 
> WebKit/chromium/src/WebViewImpl.cpp:1684
>  +      bounds.setHeight(bounds.height() - margin*2);
> Should have one space before and after '*'.
Done.

> 
> WebKit/chromium/src/WebViewImpl.cpp:1686
>  +      bounds.setWidth(bounds.width() - margin*2);
> ditto.
Done.

> 
> WebKit/chromium/src/WebViewImpl.h:136
>  +      virtual void dragSourceMovedTo(
> ditto.
Done.

> 
> WebKit/chromium/src/WebViewImpl.h:331
>  +      static const int kScrollMarginRatio = 20;
> This const definition should be moved to WebViewImpl::scrollForDragging. Also please comment why choosing value 20?
Moved to function static.
The value has left under experience. It was bad thiing...
I changed the value based on the observation on Safari behavior.

> 
> WebKit/chromium/src/WebViewImpl.h:353
>  +      void scrollForDragging(const WebPoint& clientPoint);
> The argument name can be omitted.
Done.
Comment 7 Darin Fisher (:fishd, Google) 2010-06-02 15:29:21 PDT
Comment on attachment 57634 [details]
patch v2

WebKit/chromium/public/WebView.h:173
 +      virtual void dragSourceMovedTo(const WebPoint& clientPoint, const WebPoint& screenPoint, WebDragOperation) = 0;
nit: please use the same formatting as dragSourceEndedAt

WebKit/chromium/src/WebViewImpl.cpp:1550
 +  void WebViewImpl::dragSourceMovedTo(
nit: this method impl should be placed after dragSourceEndedAt
to match the declaration order.

WebKit/chromium/src/WebViewImpl.cpp:1557
 +  
nit: only one new line between method impls.

WebKit/chromium/src/WebViewImpl.cpp:1676
 +  static IntSize distanceTo(const WebCore::IntPoint& point, const WebCore::IntRect& rect)
nit: no need for WebCore:: prefix since there is a using namespace WebCore
at the top of this file.  also, please move static helper functions up to
the top of the file after the static data declarations.

WebKit/chromium/src/WebViewImpl.cpp:1676
 +  static IntSize distanceTo(const WebCore::IntPoint& point, const WebCore::IntRect& rect)
nit: how about naming this distanceToRect?

how about this as a comment:
// Computes the distance from a point outside a rect to the nearest edge of the rect.

(I would also assert that the point is not contained within the rect if that
is indeed appropriate.)

WebKit/chromium/src/WebViewImpl.cpp:1695
 +      WebCore::FrameView* view = mainFrameImpl()->frameView();
nit: no WebCore:: prefix needed

WebKit/chromium/src/WebViewImpl.cpp:1693
 +      static const int kScrollMargin = 30;
nit: constants should not have the k prefix.  just do scrollMargin.

WebKit/chromium/src/WebViewImpl.h:136
 +      virtual void dragSourceMovedTo(const WebPoint& clientPoint, const WebPoint& screenPoint, WebDragOperation);
please list this method in the same order that it is declared
in WebView.h

WebKit/chromium/src/WebViewImpl.cpp:1690
 +  void WebViewImpl::scrollForDragging(const WebPoint& clientPoint)
this method should be defined just above hideSelectPopup() so that
the order of methods in the .cpp file matches the order of the
methods in the .h file.
Comment 8 Hajime Morrita 2010-06-02 18:12:53 PDT
Created attachment 57722 [details]
patch v3
Comment 9 Hajime Morrita 2010-06-02 18:16:59 PDT
Comment on attachment 57722 [details]
patch v3

Oops, I missed to take some feedbacks, canceling v3.
Comment 10 Hajime Morrita 2010-06-02 20:15:46 PDT
Created attachment 57729 [details]
patch v4
Comment 11 Hajime Morrita 2010-06-02 20:26:30 PDT
Hi Darin, thank you for revewing!

(In reply to comment #7)
> (From update of attachment 57634 [details])
> WebKit/chromium/public/WebView.h:173
>  +      virtual void dragSourceMovedTo(const WebPoint& clientPoint, const WebPoint& screenPoint, WebDragOperation) = 0;
> nit: please use the same formatting as dragSourceEndedAt
Done.

> 
> WebKit/chromium/src/WebViewImpl.cpp:1550
>  +  void WebViewImpl::dragSourceMovedTo(
> nit: this method impl should be placed after dragSourceEndedAt
> to match the declaration order.
Done.

> 
> WebKit/chromium/src/WebViewImpl.cpp:1557
>  +  
> nit: only one new line between method impls.
Done.

> 
> WebKit/chromium/src/WebViewImpl.cpp:1676
>  +  static IntSize distanceTo(const WebCore::IntPoint& point, const WebCore::IntRect& rect)
> nit: no need for WebCore:: prefix since there is a using namespace WebCore
> at the top of this file.  also, please move static helper functions up to
> the top of the file after the static data declarations.
Done.

> 
> WebKit/chromium/src/WebViewImpl.cpp:1676
>  +  static IntSize distanceTo(const WebCore::IntPoint& point, const WebCore::IntRect& rect)
> nit: how about naming this distanceToRect?
Renamed.

> 
> how about this as a comment:
> // Computes the distance from a point outside a rect to the nearest edge of the rect.
> 
> (I would also assert that the point is not contained within the rect if that
> is indeed appropriate.)
In some case it goes (0,0) because IntRect::contains() doesn't count its borders as its area.
So I added check instead assertion.


> 
> WebKit/chromium/src/WebViewImpl.cpp:1695
>  +      WebCore::FrameView* view = mainFrameImpl()->frameView();
> nit: no WebCore:: prefix needed
Done.

> 
> WebKit/chromium/src/WebViewImpl.cpp:1693
>  +      static const int kScrollMargin = 30;
> nit: constants should not have the k prefix.  just do scrollMargin.
Done.

> 
> WebKit/chromium/src/WebViewImpl.h:136
>  +      virtual void dragSourceMovedTo(const WebPoint& clientPoint, const WebPoint& screenPoint, WebDragOperation);
> please list this method in the same order that it is declared
> in WebView.h
Done.

> 
> WebKit/chromium/src/WebViewImpl.cpp:1690
>  +  void WebViewImpl::scrollForDragging(const WebPoint& clientPoint)
> this method should be defined just above hideSelectPopup() so that
> the order of methods in the .cpp file matches the order of the
> methods in the .h file.
Done.
Comment 12 Jian Li 2010-06-03 13:48:30 PDT
Comment on attachment 57729 [details]
patch v4

Looks good except some minor issues. Please land it with these issues addressed.

WebKit/chromium/public/WebView.h:172
 +      // Notifies the WebView that a dragging is going on.
dragging => drag

WebKit/chromium/src/WebViewImpl.cpp:845
 +      // This margin approximates Safari behavior
Please end the sentence with period.

WebKit/chromium/src/WebViewImpl.cpp:828
 +  // Computes disatances from a point to the nearest point from a rect for both x and y coordinates.
I think what Darin suggested for the comment is easy to understand. We have "the distance" represented by a pair of values, not "distances".

WebKit/chromium/src/WebViewImpl.cpp:846
 +      static const int scrollMargin = 30;
Please comment why you pick 30. Maybe you could say that this is based on the observation of ...
Comment 13 Hajime Morrita 2010-06-03 18:57:52 PDT
Committed r60651: <http://trac.webkit.org/changeset/60651>
Comment 14 Hajime Morrita 2010-06-03 19:01:12 PDT
Hi Jian, thank you for reviewing again!
The patch landing with fix you suggested.

> WebKit/chromium/public/WebView.h:172
>  +      // Notifies the WebView that a dragging is going on.
> dragging => drag
Done.

> 
> WebKit/chromium/src/WebViewImpl.cpp:845
>  +      // This margin approximates Safari behavior
> Please end the sentence with period.
Done.
> 
> WebKit/chromium/src/WebViewImpl.cpp:828
>  +  // Computes disatances from a point to the nearest point from a rect for both x and y coordinates.
> I think what Darin suggested for the comment is easy to understand. We have "the distance" represented by a pair of values, not "distances".
Done. I'm sorry that I missed this line....

> 
> WebKit/chromium/src/WebViewImpl.cpp:846
>  +      static const int scrollMargin = 30;
> Please comment why you pick 30. Maybe you could say that this is based on the observation of ...
Done.
Comment 15 Erik Arvidsson 2010-06-21 12:09:00 PDT
This does not seem to work. I'm reopening and adding a test case.
Comment 16 Erik Arvidsson 2010-06-21 12:10:27 PDT
Created attachment 59272 [details]
Test case
Comment 17 Hajime Morrita 2010-06-22 04:40:27 PDT
Hi Erik, thank you for trying this!

(In reply to comment #15)
> This does not seem to work. I'm reopening and adding a test case.
It does work for outermost frame(window), but does not work for iframe.
Safari behaves similarly. 
So I filed this new suggestion to  as a separate bug on Bug 40981 and close this.
Comment 18 Erik Arvidsson 2010-06-22 10:10:39 PDT
This bug is about Elements with scrollbars. 40981 is about iframes.
Comment 19 Hajime Morrita 2010-06-22 17:46:44 PDT
> This bug is about Elements with scrollbars. 40981 is about iframes.
Oops. So what did happen is I misunderstood the bug and fixed the non-filed bug!  
Thank you for your correction.
Comment 20 Hajime Morrita 2010-07-05 02:35:51 PDT
removing [Chromium] from summary line because this problem is not chromium-specific.
Comment 21 Hajime Morrita 2010-07-07 02:17:17 PDT
Created attachment 60710 [details]
patch v0
Comment 22 WebKit Review Bot 2010-07-07 02:19:19 PDT
Attachment 60710 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/IntRect.h:150:  More than one command on the same line  [whitespace/newline] [4]
WebCore/rendering/RenderLayer.cpp:1458:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderLayer.cpp:1471:  Missing space after ,  [whitespace/comma] [3]
WebCore/rendering/RenderLayer.cpp:1479:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/rendering/RenderLayer.cpp:1491:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/rendering/RenderLayer.cpp:1489:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/page/DragController.cpp:74:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 7 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Hajime Morrita 2010-07-07 02:53:22 PDT
Created attachment 60711 [details]
patch v1 fix style violation
Comment 24 Hajime Morrita 2010-07-07 03:09:19 PDT
Added Darin to CC because git annotate told me to do so.
Comment 25 Darin Adler 2010-07-08 11:17:50 PDT
I’d like to review this patch, but probably won’t get to it soon.

Is this always a good idea? Do other browsers do this? Any cases where doing this would be a bad thing?
Comment 26 Hajime Morrita 2010-07-08 21:35:41 PDT
Hi Darin, thank you for your feedback!

> Is this always a good idea? Do other browsers do this?
No other browser( firefox, opera, ie) doesn't support autoscroll at all, even for outermost frames.
Only safari (and latest chromium) does support it.
Drag-drop support on other browsers seems immature compared to WebKit. 
I think they don't have autoscroll yet because they don't have enough drag-drop support yet.
So we could pioneer in this area.

Actually, some desktop applications including (non-internet) explorer 
also support autoscroll by dragging.

> Any cases where doing this would be a bad thing?
Users could trigger scrolling accidentally if the autoscroll criteria is too sensitive.
So this patch uses a timer to delay triggering the autoscroll (as Cocoa's autoscroll does).
With such delay, chances for accidental scrolling can be minimized.

Just FYI, here is a viewpoint from chromium land:
We have the bookmark manager implemented in HTML, that heavily uses drag-and-drop.
This feature is originally requested for improving its experience.

Regards.
Comment 27 Hajime Morrita 2010-09-08 03:50:25 PDT
Created attachment 66868 [details]
sync to latest
Comment 28 Hajime Morrita 2010-10-25 01:15:19 PDT
Created attachment 71721 [details]
Patch
Comment 29 Hajime Morrita 2010-10-25 01:18:00 PDT
Could anyone give me a review?
This scroll-following-drag is vital for high-density UI with drag operation.
Comment 30 Hajime Morrita 2011-01-25 21:29:48 PST
Comment on attachment 71721 [details]
Patch

Got obsolete, giving up at now.
Comment 31 yosin 2012-11-30 03:05:42 PST
Created attachment 176933 [details]
WIP Patch 2.0
Comment 32 Hajime Morrita 2012-11-30 15:26:41 PST
Comment on attachment 176933 [details]
WIP Patch 2.0

This one looks much simpler than the last version that I wrote. This is great!
One caveat to this patch is that we have too much rendering related logic in EventHandler, which violates layering.
It would be better if we move them to somewhere in rendering/
Comment 33 yosin 2012-12-04 23:38:00 PST
Created attachment 177680 [details]
Patch 2.0-1
Comment 34 yosin 2012-12-05 01:38:52 PST
Comment on attachment 177680 [details]
Patch 2.0-1

Could you review this patch?
Thanks in advance.
Comment 35 Build Bot 2012-12-05 01:47:28 PST
Comment on attachment 177680 [details]
Patch 2.0-1

Attachment 177680 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15153327

New failing tests:
fast/events/drag-and-drop-dataTransfer-types-nocrash.html
fast/events/drag-and-drop-autoscroll.html
Comment 36 yosin 2012-12-05 02:03:53 PST
Comment on attachment 177680 [details]
Patch 2.0-1

Oops, failed on Mac test. Investigating...
Comment 37 yosin 2012-12-05 23:19:19 PST
Created attachment 177950 [details]
Patch 2.0-2
Comment 38 yosin 2012-12-06 01:15:18 PST
Comment on attachment 177950 [details]
Patch 2.0-2

Could you review this patch?
Thanks in advance.
Comment 39 Hajime Morrita 2012-12-11 20:46:49 PST
Comment on attachment 177950 [details]
Patch 2.0-2

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

The approach looks good. Let's take this as an opportunity to cleanup auto-scrolling code.

> Source/WebCore/page/EventHandler.cpp:136
> +// Delay time in second for start autoscroll if pointer is in border edge of scrollable element.

can be static since it's referred only from this file.

> Source/WebCore/page/EventHandler.cpp:137
> +const double autoscrollDelay = 0.2;

Ditto.

> Source/WebCore/page/EventHandler.cpp:141
> +const int autoscrollBeltSize = 20;

same.
Comment 40 Hajime Morrita 2012-12-11 20:51:28 PST
Comment on attachment 177950 [details]
Patch 2.0-2

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

> Source/WebCore/page/EventHandler.h:408
> +    IntPoint m_autoscrollReferencePoint;

It's sad to see EventHandler getting fat.
It looks we already have bunch of autoscroll related state and we're adding more.
Why not factor these out to a class which is responsible for auto scrolling?

> Source/WebCore/rendering/RenderObject.h:275
> +    virtual bool canAutoscroll() const { return false; }

I don't think this need to be virtual. It can be a method of RenderBox.

> Source/WebCore/rendering/RenderObject.h:276
> +    RenderObject* findAutoscrollableRenderer();

For that, this should return RenderBox.
I think this can be a static method of RenderBox so that autoscrolling related code
can be stay in the same class.
Comment 41 yosin 2012-12-14 01:41:30 PST
Created attachment 179448 [details]
Patch 3.0
Comment 42 yosin 2012-12-14 01:42:37 PST
Comment on attachment 179448 [details]
Patch 3.0

Could you review this patch?
Thanks in advance.
Comment 43 Hajime Morrita 2013-01-07 20:33:31 PST
Comment on attachment 179448 [details]
Patch 3.0

Could you clarify when an autoscrol of the specific type starts and ends?
AutoscrollForDragAndDrop is hidden in ambiguous "update" function. That isn't good.
We need start and hopefully end functions.
Comment 44 yosin 2013-01-07 21:54:59 PST
Created attachment 181638 [details]
Patch-3.0.1
Comment 45 yosin 2013-01-07 22:01:26 PST
Comment on attachment 181638 [details]
Patch-3.0.1

Could you review this patch?
Thanks in advance.

= Changes since the last review =
Changes AutoscrollController::updateDragAndDrop to be clear, it does

- If new drop target is not autoscrollable, stops autoscroll.
- If autoscroll is in progress, update autoscroll renderer and update event start time if needed.
- Otherwise, start autoscroll for DnD.
Comment 46 Hajime Morrita 2013-01-08 00:27:03 PST
Comment on attachment 181638 [details]
Patch-3.0.1

OK.
Comment 47 WebKit Review Bot 2013-01-08 00:59:17 PST
Comment on attachment 181638 [details]
Patch-3.0.1

Clearing flags on attachment: 181638

Committed r139044: <http://trac.webkit.org/changeset/139044>
Comment 48 WebKit Review Bot 2013-01-08 00:59:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 49 Ryosuke Niwa 2013-01-08 12:08:20 PST
The test added by this patch is failing on Mac:
http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r139087%20(4563)/fast/events/drag-and-drop-autoscroll-pretty-diff.html

--- /Volumes/Data/slave/mountainlion-release-tests-wk2/build/layout-test-results/fast/events/drag-and-drop-autoscroll-expected.txt
+++ /Volumes/Data/slave/mountainlion-release-tests-wk2/build/layout-test-results/fast/events/drag-and-drop-autoscroll-actual.txt
@@ -3,6 +3,6 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS scrollable.scrollTop > 0
+FAIL No autoscroll
 For manual testing, drag and drop "Drop Me" to "Scrollable" area.
Comment 50 Ryosuke Niwa 2013-01-08 12:14:34 PST
The test is failing on all but chromium port:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=fast%2Fevents%2Fdrag-and-drop-autoscroll.html

Is this supposed to work only in chromium? If so, why was the test added to fast/events instead of chromium/fast/events?

If not, then I would like to know why you didn't fix non-chromium ports.
Comment 51 Ryosuke Niwa 2013-01-08 12:21:26 PST
If I don't get any response from either one of you in a timely mannar, I'm going to revert this patch.
Comment 52 Erik Arvidsson 2013-01-08 13:06:51 PST
(In reply to comment #51)
> If I don't get any response from either one of you in a timely mannar, I'm going to revert this patch.

This doesn't look Chromium specific.

Morrita is not in your time zone so please be patient.
Comment 53 Hajime Morrita 2013-01-08 16:12:17 PST
This patch started from Chromium-specific approach but
changed direction during updates.

Anyway, It shoudln't break any tests. I'll rollout this.
Comment 54 WebKit Review Bot 2013-01-08 16:14:27 PST
Re-opened since this is blocked by bug 106395
Comment 55 Hajime Morrita 2013-01-08 16:21:27 PST
(In reply to comment #54)
> Re-opened since this is blocked by bug 106395
We can just mark it fail.
Comment 56 Simon Fraser (smfr) 2013-01-11 14:44:45 PST
This patch is caused unwanted scroll behavior on Mac. There are several issues:
1. If I drag a URL over a page, it will autoscroll when near the top and bottom for no useful reason: there are no drop targets anywhere on the page, even out of view.
2. The page seems to get stuck in "autoscroll" mode even when the drag has finished; scrolling the page manually will cause it to drift in one direction, or fight the user scrolling.

I'm going to revert this change.
Comment 57 WebKit Review Bot 2013-01-11 14:47:12 PST
Re-opened since this is blocked by bug 106702
Comment 58 Simon Fraser (smfr) 2013-01-11 14:59:28 PST
Steps to reproduce the bug that caused the rollout:

1. Go to a website in a Mac WebKit2 view
2. Drag something into the window (e.g. text from a text editor)
3. Some glitches happens: the content moves up and down real fast
4. Scroll to the middle of the page and release
5. The content slowly scroll back to the top.
Comment 59 Avi Drissman 2013-01-11 16:46:31 PST
Was this rolled out? This causes autoscroll issues in Chromium:

https://code.google.com/p/chromium/issues/detail?id=169630

Repro steps there, though it seems that you see them in other ports too.
Comment 60 Ryosuke Niwa 2013-01-11 16:47:59 PST
(In reply to comment #59)
> Was this rolled out? This causes autoscroll issues in Chromium:
> 
> https://code.google.com/p/chromium/issues/detail?id=169630
> 
> Repro steps there, though it seems that you see them in other ports too.

Yes, it has been rolled out.
Comment 61 Ojan Vafai 2013-01-12 10:05:53 PST
(In reply to comment #56)
> This patch is caused unwanted scroll behavior on Mac. There are several issues:
> 1. If I drag a URL over a page, it will autoscroll when near the top and bottom for no useful reason: there are no drop targets anywhere on the page, even out of view.

This particular bit seems fine to me. I prefer the consistency of always doing the scrolling over only doing so when there are drop targets. Perhaps this is a UI decision different ports will want to handle differently though. I suppose we should check with our UI folks to get an official word on what behavior we'd like.

If we need to, we could put it behind a setting with a FIXME to enable it once there's a setting to only scroll when there are drop targets.

I wonder if we should also have some heuristic to not scroll when you initially drag in, but only when you move back to the edge of the scrollable region to avoid scrolling if you are aiming for a currently visible drop target.

There were clearly other bugs with this patch, so rolling out makes sense.
Comment 62 Hajime Morrita 2013-01-14 16:20:46 PST
Oops. I'm sorry to be late here. Thanks for taking care of this.

> 1. If I drag a URL over a page, it will autoscroll when near the top and bottom for no useful reason: there are no drop targets anywhere on the page, even out of view.
> 2. The page seems to get stuck in "autoscroll" mode even when the drag has finished; scrolling the page manually will cause it to drift in one direction, or fight the user scrolling.

At least 2 is obviously a bug and need to be fixed.
For 1, it might need some heuristic as Ojan said.
It can be platform/client-specific
and always return no-scroll by default.
Comment 63 yosin 2013-01-15 00:31:13 PST
Created attachment 182711 [details]
Patch-3.0.2
Comment 64 yosin 2013-01-15 00:39:39 PST
Comment on attachment 182711 [details]
Patch-3.0.2

Could you review this patch?
Thanks in advance.

This patch fixes non-stopping autoscroll issue. The root cause is forgetting call of stop autscroll timer at cancellation of drag-and-drop, e.g. pressing ESCAPE key, drag off the window from another process's drag source.

= Changes since the last review =
* Call stopAutoscrollTimer() at EventHandler::clearDragState() which called after drop or cancel of drag-and-drop.
* Introduce ChromeClient::shouldAutoscrollForDragAndDrop() for enabling drag-and-drop autoscroll by each port rater than all ports. Note: Chromium port enables.
* Layout test drag-and-drop-autosccroll.html checks stopping of autoscroll after drag-and-drop cancellation.
Comment 65 WebKit Review Bot 2013-01-15 00:43:21 PST
Attachment 182711 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Tools/DumpRenderTree/chromium/TestRunner/src/KeyCodeMapping.h:39:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 66 Build Bot 2013-01-15 00:59:12 PST
Comment on attachment 182711 [details]
Patch-3.0.2

Attachment 182711 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15871816
Comment 67 yosin 2013-01-15 01:15:06 PST
Created attachment 182718 [details]
Patch-3.0.3
Comment 68 yosin 2013-01-15 01:18:22 PST
Comment on attachment 182718 [details]
Patch-3.0.3

Could you review this patch?
Thanks in advance.

* Mac compilation failure was fixed, unused parameter in shouldAutoscrollForDragAndDrop() declaration.
* Style failure for VKEY_ESCAPE is a tool issue, https://bugs.webkit.org/show_bug.cgi?id=106881

This patch fixes non-stopping autoscroll issue. The root cause is forgetting call of stop autscroll timer at cancellation of drag-and-drop, e.g. pressing ESCAPE key, drag off the window from another process's drag source.

= Changes since the last review =
* Call stopAutoscrollTimer() at EventHandler::clearDragState() which called after drop or cancel of drag-and-drop.
* Introduce ChromeClient::shouldAutoscrollForDragAndDrop() for enabling drag-and-drop autoscroll by each port rater than all ports. Note: Chromium port enables.
* Layout test drag-and-drop-autosccroll.html checks stopping of autoscroll after drag-and-drop cancellation.
Comment 69 WebKit Review Bot 2013-01-15 01:21:10 PST
Attachment 182718 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Tools/DumpRenderTree/chromium/TestRunner/src/KeyCodeMapping.h:39:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 70 yosin 2013-01-16 20:43:02 PST
Created attachment 183109 [details]
Patch-3.0.4
Comment 71 yosin 2013-01-16 20:44:50 PST
Comment on attachment 183109 [details]
Patch-3.0.4

Could you review this patch?
Thanks in advance.

= Changes since the last patch =
* Add skip entry for drag-and-drop-autoscroll.html into TestExpectations other than Chromium ports.

= Changes since the last review =
* Mac compilation failure was fixed, unused parameter in shouldAutoscrollForDragAndDrop() declaration.
* Style failure for VKEY_ESCAPE is a tool issue, https://bugs.webkit.org/show_bug.cgi?id=106881

This patch fixes non-stopping autoscroll issue. The root cause is forgetting call of stop autscroll timer at cancellation of drag-and-drop, e.g. pressing ESCAPE key, drag off the window from another process's drag source.

= Changes since the last review =
* Call stopAutoscrollTimer() at EventHandler::clearDragState() which called after drop or cancel of drag-and-drop.
* Introduce ChromeClient::shouldAutoscrollForDragAndDrop() for enabling drag-and-drop autoscroll by each port rater than all ports. Note: Chromium port enables.
* Layout test drag-and-drop-autosccroll.html checks stopping of autoscroll after drag-and-drop cancellation.
Comment 72 WebKit Review Bot 2013-01-16 20:47:52 PST
Attachment 183109 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Tools/DumpRenderTree/chromium/TestRunner/src/KeyCodeMapping.h:39:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 73 yosin 2013-01-18 01:33:05 PST
Created attachment 183400 [details]
Patch 3.0.5
Comment 74 yosin 2013-01-18 01:34:49 PST
Comment on attachment 183400 [details]
Patch 3.0.5

Could you review this patch?
Thanks in advance.

This is rebase version of previous patch.
Comment 75 WebKit Review Bot 2013-01-18 01:36:46 PST
Attachment 183400 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Tools/DumpRenderTree/chromium/TestRunner/src/KeyCodeMapping.h:39:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 76 Hajime Morrita 2013-01-18 01:43:56 PST
Comment on attachment 183400 [details]
Patch 3.0.5

Let's try this again.
Comment 77 WebKit Review Bot 2013-01-20 17:30:36 PST
Comment on attachment 183400 [details]
Patch 3.0.5

Clearing flags on attachment: 183400

Committed r140286: <http://trac.webkit.org/changeset/140286>
Comment 78 WebKit Review Bot 2013-01-20 17:30:46 PST
All reviewed patches have been landed.  Closing bug.