Bug 39725 – Dragging over an element with scrollbars should scroll the element when dragging near edges

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Hajime Morrita]

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

[Hajime Morrita]

Created attachment 57296 [details]
patch v0

[WebKit Review Bot]

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.

[Hajime Morrita]

Created attachment 57298 [details]
patch v1 - fix style violation

[Jian Li]

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.

[Hajime Morrita]

Created attachment 57634 [details]
patch v2

[Hajime Morrita]

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.

[Darin Fisher (:fishd, Google)]

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.

[Hajime Morrita]

Created attachment 57722 [details]
patch v3

[Hajime Morrita]

Comment on attachment 57722 [details]
patch v3

Oops, I missed to take some feedbacks, canceling v3.

[Hajime Morrita]

Created attachment 57729 [details]
patch v4

[Hajime Morrita]

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.

[Jian Li]

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 ...

[Hajime Morrita]

Committed r60651: <http://trac.webkit.org/changeset/60651>

[Hajime Morrita]

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.

[Erik Arvidsson]

This does not seem to work. I'm reopening and adding a test case.

[Erik Arvidsson]

Created attachment 59272 [details]
Test case

[Hajime Morrita]

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.

[Erik Arvidsson]

This bug is about Elements with scrollbars. 40981 is about iframes.

[Hajime Morrita]

> 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.

[Hajime Morrita]

removing [Chromium] from summary line because this problem is not chromium-specific.

[Hajime Morrita]

Created attachment 60710 [details]
patch v0

[WebKit Review Bot]

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.

[Hajime Morrita]

Created attachment 60711 [details]
patch v1 fix style violation

[Hajime Morrita]

Added Darin to CC because git annotate told me to do so.

[Darin Adler]

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?

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 66868 [details]
sync to latest

[Hajime Morrita]

Created attachment 71721 [details]
Patch

[Hajime Morrita]

Could anyone give me a review?
This scroll-following-drag is vital for high-density UI with drag operation.

[Hajime Morrita]

Comment on attachment 71721 [details]
Patch

Got obsolete, giving up at now.

Created attachment 176933 [details]
WIP Patch 2.0

[Hajime Morrita]

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/

Created attachment 177680 [details]
Patch 2.0-1

Comment on attachment 177680 [details]
Patch 2.0-1

Could you review this patch?
Thanks in advance.

[Build Bot]

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 on attachment 177680 [details]
Patch 2.0-1

Oops, failed on Mac test. Investigating...

Created attachment 177950 [details]
Patch 2.0-2

Comment on attachment 177950 [details]
Patch 2.0-2

Could you review this patch?
Thanks in advance.

[Hajime Morrita]

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.

[Hajime Morrita]

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.

Created attachment 179448 [details]
Patch 3.0

Comment on attachment 179448 [details]
Patch 3.0

Could you review this patch?
Thanks in advance.

[Hajime Morrita]

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.

Created attachment 181638 [details]
Patch-3.0.1

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.

[Hajime Morrita]

Comment on attachment 181638 [details]
Patch-3.0.1

OK.

[WebKit Review Bot]

Comment on attachment 181638 [details]
Patch-3.0.1

Clearing flags on attachment: 181638

Committed r139044: <http://trac.webkit.org/changeset/139044>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

If I don't get any response from either one of you in a timely mannar, I'm going to revert this patch.

[Erik Arvidsson]

(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.

[Hajime Morrita]

This patch started from Chromium-specific approach but
changed direction during updates.

Anyway, It shoudln't break any tests. I'll rollout this.

[WebKit Review Bot]

Re-opened since this is blocked by bug 106395

[Hajime Morrita]

(In reply to comment #54)
> Re-opened since this is blocked by bug 106395
We can just mark it fail.

[Simon Fraser (smfr)]

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.

[WebKit Review Bot]

Re-opened since this is blocked by bug 106702

[Simon Fraser (smfr)]

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.

[Avi Drissman]

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.

[Ryosuke Niwa]

(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.

[Ojan Vafai]

(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.

[Hajime Morrita]

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.

Created attachment 182711 [details]
Patch-3.0.2

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.

[WebKit Review Bot]

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.

[Build Bot]

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

Created attachment 182718 [details]
Patch-3.0.3

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.

[WebKit Review Bot]

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.

Created attachment 183109 [details]
Patch-3.0.4

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.

[WebKit Review Bot]

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.

Created attachment 183400 [details]
Patch 3.0.5

Comment on attachment 183400 [details]
Patch 3.0.5

Could you review this patch?
Thanks in advance.

This is rebase version of previous patch.

[WebKit Review Bot]

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.

[Hajime Morrita]

Comment on attachment 183400 [details]
Patch 3.0.5

Let's try this again.

[WebKit Review Bot]

Comment on attachment 183400 [details]
Patch 3.0.5

Clearing flags on attachment: 183400

Committed r140286: <http://trac.webkit.org/changeset/140286>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.