Middle-click panning should be springloaded while dragging

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

There are two panning modes in Firefox and IE: springloaded and non-springloaded:

Springloaded: Panning only occurs while the middle mouse button is down. This is triggered by a middle-mouse-button-drag.

Non-springloaded: Panning occurs between the first middle-click and a subsequent middle-click. This is triggered by a middle-click.

WebKit currently only implements the non-springloaded behavior. We need to implement the springloaded behavior.

To reproduce:

1. Go to any web page
2. Resize the Safari window so that the page has scroll bars
3. Press the middle mouse button and hold it down
4. Move the mouse around while holding down the middle mouse button
5. Release the middle mouse button

After step 5, panning mode should be aborted. Instead, panning mode continues until you click the middle mouse button again.

This is a difference from middle-click panning behavior in Firefox and IE.

[Adam Roben (:aroben)]

<rdar://problem/6310538>

[Loren Segal]

Created attachment 27098 [details]
Turn off pan scrolling when middle mouse button is released during a drag gesture

Had to refactor a little of setPanScrollCursor() to detect if the mouse is scrolling-- is there a better way to do this?

[Adam Roben (:aroben)]

Comment on attachment 27098 [details]
Turn off pan scrolling when middle mouse button is released during a drag gesture

Thanks for the patch!

> +#if ENABLE(PAN_SCROLLING)
> +        if (m_panScrollInProgress && mouseEvent.button() == MiddleButton  
> +            && selectPanScrollCursor().impl().type() != middlePanningCursor().impl().type()) {

I don't think this will compile on all platforms (most platforms' PlatformCursor types do not have a type() function).

Loren's going to refactor this patch a bit to add something like a panScrollingDirection() function that can be used both to decide the cursor and to decide whether to do spring-loaded scrolling.

[Loren Segal]

Created attachment 27128 [details]
Turn off pan scroll when middle button is released in a drag gesture

[Adam Roben (:aroben)]

Comment on attachment 27128 [details]
Turn off pan scroll when middle button is released in a drag gesture

> Index: ChangeLog
> ===================================================================
> --- ChangeLog	(revision 40333)
> +++ ChangeLog	(working copy)
> @@ -1,3 +1,12 @@
> [phone]  Loren Segal  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Turn off pan scrolling when middle mouse button is released during a mouse drag gesture.

You should mention the bug title and URL in your ChangeLog.

> +
> +        * page/EventHandler.cpp:
> +        * page/EventHandler.h:

The prepare-ChangeLog script will insert the names of the functions you modified. You should make a comment about each function you modified explaining your change.

> +++ page/EventHandler.h	(working copy)
> @@ -216,8 +216,22 @@ private:
>      void handleKeyboardSelectionMovement(KeyboardEvent*);
>      
>      Cursor selectCursor(const MouseEventWithHitTestResults&, Scrollbar*);
> +    Cursor selectPanScrollCursor();
>      void setPanScrollCursor();

I don't think we need selectPanScrollCursor anymore. You can just put the code into setPanScrollCursor. That should lead to a smaller diff.

> @@ -299,6 +313,7 @@ private:
>  
>      IntPoint m_panScrollStartPos;
>      bool m_panScrollInProgress;
> +    bool m_panScrollIsScrolling;

The name "m_panScrollIsScrolling" is a little confusing when taken together with m_panScrollInProgress. I'd suggest something like m_panScrollHasScrolled or m_panScrollHasScrolledSinceStarting.

Let's take care of these last few issues and get this landed!

[Loren Segal]

Created attachment 27160 [details]
Middle click panning

[Adam Roben (:aroben)]

Comment on attachment 27160 [details]
Middle click panning

> +#if ENABLE(PAN_SCROLLING)
> +    if (m_panScrollInProgress && !m_panScrollHasScrolled && panScrollDirection() != PanScrollNone) {
> +        m_panScrollHasScrolled = true;
> +    }
> +#endif

We normally leave off braces around single-line ifs. I can fix this up before landing the patch.

> +    PanScrollDirection panScrollDirection();

I think this can be a const member function. I'll fix that, too.

Looks great!

[Adam Roben (:aroben)]

Comment on attachment 27160 [details]
Middle click panning

I tried applying this patch, but now pan-scrolling seems buggy. I haven't yet gotten a springloaded scroll to scroll the page at all. Worse, after trying a springloaded scroll, non-springloaded scrolls seem to stay broken for a while, too.

[Loren Segal]

(In reply to comment #8)
> (From update of attachment 27160 [details] [review])
> I tried applying this patch, but now pan-scrolling seems buggy. I haven't yet
> gotten a springloaded scroll to scroll the page at all. Worse, after trying a
> springloaded scroll, non-springloaded scrolls seem to stay broken for a while,
> too.

Could you explain the issue in a little more detail? I applied this patch on Chrome under Windows and it works just fine here. 

I also compiled WebKit under OS X (and enabling PAN_SCROLLING) and the release code worked. I noticed that under OS X the pan behaviour was a little wonky (scroll directions seem to work backwards), so I unapplied the patch to see if it was my code but the same behaviour remained without my patch. Maybe what you're seeing is a pre-existing/unrelated issue?

[Itai]

Created attachment 29499 [details]
WebKit Option For Springloaded Autoscroll

[Darin Fisher (:fishd, Google)]

Comment on attachment 29499 [details]
WebKit Option For Springloaded Autoscroll

indentation should be 4 white spaces.  please review the webkit style guide: http://webkit.org/coding/coding-style.html

[Itai]

Created attachment 29506 [details]
Updated version of previous patch

[Itai]

(In reply to comment #11)
> (From update of attachment 29499 [details] [review])
> indentation should be 4 white spaces.  please review the webkit style guide:
> http://webkit.org/coding/coding-style.html
> 

Thanks for the review. BTW, it sounded like I did not follow the style guide at all, I only found one line incorrectly indented line. I would appreciate it if the comments told me where to look next time. I've uploaded a correct patch for your review.

[Darin Fisher (:fishd, Google)]

Comment on attachment 29506 [details]
Updated version of previous patch

> Index: WebCore/ChangeLog
...
> [phone]  Itai Danan  <set EMAIL_ADDRESS environment variable>
> +        As per Bug 21794 (https://bugs.webkit.org/show_bug.cgi?id=21794),
> +	added an option for springloaded panscroll instead of sticky
> +	scroll. To understand why this is required see the discussion in
> +	issue 24722 (https://bugs.webkit.org/show_bug.cgi?id=24722).	

^^^ please replace tab characters with spaces.


> Index: WebCore/page/EventHandler.cpp
...
>      m_currentMousePosition = mouseEvent.pos();
>  
> +    if (!m_frame->settings()->stickyPanScroll())
> +      if (m_frame->page() && m_frame->page()->mainFrame()->eventHandler()->panScrollInProgress() || m_autoscrollInProgress) {
> +        stopAutoscrollTimer();
> +        invalidateClick();
> +        return true;
> +      }
> +

^^^ please change to use 4 space indentation

[Itai]

Created attachment 29508 [details]
Version with corrected tabs and indentation

[Itai]

All review comments were addressed (see latest patch).

[Darin Fisher (:fishd, Google)]

Comment on attachment 29508 [details]
Version with corrected tabs and indentation

> Index: WebCore/ChangeLog
> [phone]  Itai Danan  <[e-mail]>
> +        As per Bug 21794 (https://bugs.webkit.org/show_bug.cgi?id=21794),
> +        added an option for springloaded panscroll instead of sticky
> +        scroll. To understand why this is required see the discussion in
> +        issue 24722 (https://bugs.webkit.org/show_bug.cgi?id=24722).
> +
> +        Reviewed by NOBODY (OOPS!).
> +

please put the description below the "reviewed by" and please add a new line
after the line with your name.


> Index: WebCore/page/Settings.h
...
>  
> +        void setStickyPanScroll(bool);
> +        bool stickyPanScroll() const { return m_stickyPanScroll; }

please add a comment explaining what 'sticky pan scrolling' is

[Eric Seidel (no email)]

Comment on attachment 29508 [details]
Version with corrected tabs and indentation

Darin Fisher is not the only capable reviewer here, and may not even be the best one.  Better to leave the review w/o a specific reviewer.

[Maciej Stachowiak]

I'd like some input from Jon, if possible, since he reviewed the patch on the cited related bug.

[Jon Honeycutt]

Comment on attachment 29508 [details]
Version with corrected tabs and indentation

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 42539)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,20 @@
> [phone]  Itai Danan  <[e-mail]>
> +        As per Bug 21794 (https://bugs.webkit.org/show_bug.cgi?id=21794),
> +        added an option for springloaded panscroll instead of sticky
> +        scroll. To understand why this is required see the discussion in
> +        issue 24722 (https://bugs.webkit.org/show_bug.cgi?id=24722).
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * page/EventHandler.cpp:
> +        (WebCore::EventHandler::handleMousePressEvent):
> +        (WebCore::EventHandler::handleMouseReleaseEvent):
> +        * page/Settings.cpp:
> +        (WebCore::Settings::Settings):
> +        (WebCore::Settings::setStickyPanScroll):
> +        * page/Settings.h:
> +        (WebCore::Settings::stickyPanScroll):
> +

Please fill out your ChangeLog to describe the changes you are making to each function, unless the change is something very trivial, such as adding a getter or setter. It's usually safe to be more rather than less verbose - see other entries in the ChangeLog for examples.

> Index: WebCore/page/EventHandler.cpp
> ===================================================================
> --- WebCore/page/EventHandler.cpp	(revision 42539)
> +++ WebCore/page/EventHandler.cpp	(working copy)
> @@ -1102,11 +1102,12 @@ bool EventHandler::handleMousePressEvent
>  
>  #if ENABLE(PAN_SCROLLING)
>      Page* page = m_frame->page();
> -    if (page && page->mainFrame()->eventHandler()->panScrollInProgress() || m_autoscrollInProgress) {
> -        stopAutoscrollTimer();
> -        invalidateClick();
> -        return true;
> -    }
> +    if (m_frame->settings()->stickyPanScroll())
> +        if (page && page->mainFrame()->eventHandler()->panScrollInProgress() || m_autoscrollInProgress) {
> +            stopAutoscrollTimer();
> +            invalidateClick();
> +            return true;
> +        }

WebKit style requires multiline control clauses to have braces, and you should join these into one statement:

if (m_frame->settings()->stickyPanScroll() &&
    (page && page->mainFrame()->eventHandler()->panScrollInProgress() || m_autoscrollInProgress)) {

>      
>      if (mouseEvent.button() == MiddleButton && !mev.isOverLink()) {
>          RenderObject* renderer = mev.targetNode()->renderer();
> @@ -1348,6 +1349,13 @@ bool EventHandler::handleMouseReleaseEve
>      m_mousePressed = false;
>      m_currentMousePosition = mouseEvent.pos();
>  
> +    if (!m_frame->settings()->stickyPanScroll())
> +        if (m_frame->page() && m_frame->page()->mainFrame()->eventHandler()->panScrollInProgress() || m_autoscrollInProgress) {
> +            stopAutoscrollTimer();
> +            invalidateClick();
> +            return true;
> +        }
> +

Same comment as above.

> Index: WebCore/page/Settings.cpp
> ===================================================================
> --- WebCore/page/Settings.cpp	(revision 42539)
> +++ WebCore/page/Settings.cpp	(working copy)
> @@ -90,6 +90,7 @@ Settings::Settings(Page* page)
>      , m_usesEncodingDetector(false)
>      , m_maximumDecodedImageSize(std::numeric_limits<size_t>::max())
>      , m_allowScriptsToCloseWindows(false)
> +    , m_stickyPanScroll(true)
>  {
>      // A Frame may not have been created yet, so we initialize the AtomicString 
>      // hash before trying to use it.
> @@ -435,4 +436,9 @@ void Settings::setAllowScriptsToCloseWin
>      m_allowScriptsToCloseWindows = allowScriptsToCloseWindows;
>  }
>  
> +void Settings::setStickyPanScroll(bool stickyPanScroll)
> +{
> +    m_stickyPanScroll = stickyPanScroll;
> +}
> +

We usually place the definition of setters in the header file.

>  } // namespace WebCore
> Index: WebCore/page/Settings.h
> ===================================================================
> --- WebCore/page/Settings.h	(revision 42539)
> +++ WebCore/page/Settings.h	(working copy)
> @@ -214,6 +214,9 @@ namespace WebCore {
>          void setAllowScriptsToCloseWindows(bool);
>          bool allowScriptsToCloseWindows() const { return m_allowScriptsToCloseWindows; }
>  
> +        void setStickyPanScroll(bool);
> +        bool stickyPanScroll() const { return m_stickyPanScroll; }
> +

I think this would be more clear if you added the word "uses", e.g. usesStickyPanScroll().

Thanks for the patch! r- for the issues above, but other than the style issues, it looks good to me.

[Itai]

Created attachment 30713 [details]
Comments by John Honeycutt addressed

[Jon Honeycutt]

Comment on attachment 30713 [details]
Comments by John Honeycutt addressed

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 44197)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,23 @@
> [phone]  Itai Danan  <[e-mail]>
> +
> +        Reviewed by [e-mail].

We usually use the reviewer's name; I'm Jon Honeycutt.

> +
> +        As per Bug 21794 (https://bugs.webkit.org/show_bug.cgi?id=21794),
> +	added an option for springloaded panscroll instead of sticky
> +	scroll. To understand why this is required see the discussion in
> +	issue 24722 (https://bugs.webkit.org/show_bug.cgi?id=24722).
> +
> +        * page/EventHandler.cpp:
> +        (WebCore::EventHandler::handleMousePressEvent): Use autoscroll behavior flag to determine
> +	if autoscroll needs to be released on this event.
> +        (WebCore::EventHandler::handleMouseReleaseEvent):  Use autoscroll behavior flag to determine
> +	if autoscroll needs to be released on this event (reverse logic of for Press event).

I think you added some tabs here. WebKit uses 4 spaces for indentation.

r=me if you fix the tabs. Thanks for the patch, Itai!

[Itai]

Created attachment 30716 [details]
Tabs removed from ChangeLog

[Adam Roben (:aroben)]

(In reply to comment #22)
> (From update of attachment 30713 [details] [review])
> > Index: WebCore/ChangeLog
> > ===================================================================
> > --- WebCore/ChangeLog	(revision 44197)
> > +++ WebCore/ChangeLog	(working copy)
> > @@ -1,3 +1,23 @@
> > [phone]  Itai Danan  <[e-mail]>
> > +
> > +        Reviewed by [e-mail].
> 
> We usually use the reviewer's name; I'm Jon Honeycutt.

But we also normally don't put any reviewer's name in patches that are up for review. The committer fills in the reviewer's name when it's time to commit the patch. The "NOBODY (OOPS!)" text should be left in, because a pre-commit hook searches for this to make sure that a reviewer was listed.

[David Levin]

Assign to levin for landing.

[Adam Roben (:aroben)]

Comment on attachment 30716 [details]
Tabs removed from ChangeLog

This patch really should have been attached to bug 24791. Maybe Dave can update the ChangeLog to mention that bug when landing, instead of mentioning this one.

[David Levin]

Note to levin need to close the other bug and leave this one open when landing the patch.

[Peter Kasting]

I don't understand what Itai's patch does, or why it was r+ed and landed.

It looks like it adds an option to toggle between "always do sticky autoscroll" (the old behavior) and "always do spring-loaded autoscroll".

If this is in fact what it does, no client wants this.  Certainly what Chromium wants (and what I suspect Safari will want) is more what Loren's patch aimed for: spring-loaded autoscroll on drag, sticky otherwise.

Can someone explain what benefit this patch has?  If not, I intend to revert it after a reasonable amount of time has passed.

[Peter Kasting]

Furthermore, the patch justifies itself by referring to bug 24791, but that bug is about preffing-off autoscroll entirely.

This whole thing is just confused-looking.

[David Levin]

Comment on attachment 30716 [details]
Tabs removed from ChangeLog

Removing the r+ to move this out of the commit queue.  

For better or worse, it was committed as http://trac.webkit.org/changeset/44276

Itai, please don't mark your own patches as r+.  I was able to piece together why this patch was an r+ (without your r+) so I committed it on that basis.

[David Levin]

Giving it back to unassigned since the patch has been landed.

[David Levin]

Comment on attachment 30713 [details]
Comments by John Honeycutt addressed

Removing jhoneycutt's r+ to move this out of the commit queue.  The patch with the r+ was landed as noted.

[Itai]

Sorry for not doing the right thing. Unfortunately, I am still confused as
what some of this means. The patch was r+ by Jon Honeycutt when he said to
fix some tabs and reupload. Upon the upload, I left the r+. I did not know
that the status could be set to "none" which is what it is at now.

I'm not going touch anything other flags now for fear of not doing the
right thing but someone should probably resolve sine it has been implemented.
It is the responsibility of the browsers to use the preference as needed.

(In reply to comment #30)
> (From update of attachment 30716 [details] [review])
> Removing the r+ to move this out of the commit queue.  
> 
> For better or worse, it was committed as http://trac.webkit.org/changeset/44276
> 
> Itai, please don't mark your own patches as r+.  I was able to piece together
> why this patch was an r+ (without your r+) so I committed it on that basis.
> 

[Adam Roben (:aroben)]

(In reply to comment #33)
> someone should probably resolve sine it has been implemented.
> It is the responsibility of the browsers to use the preference as needed.

Itai, your patch should have been attached to bug 24791, not this bug. Bug 24791 was about implementing a preference, and has been marked RESOLVED/FIXED since your patch landed.

This bug is about a different issue: performing the sequence "middle down, middle up, move" should leave you in pan-to-scroll mode (as it does today), but performing the sequence "middle down, move, middle up" should stop the pan-to-scroll (this is the "springloaded" behavior).

The preference is an entirely separate issue (which is why it's represented by a different bug, even though your patches ended up mistakenly attached to this one).

[Itai]

See inline:

(In reply to comment #28)
> I don't understand what Itai's patch does, or why it was r+ed and landed.

Not sure where the confusion comes from but clearly there is some, perhaps because a number 
of bugs sound very similar. The title of this one is "Middle-click panning should be springloaded
while dragging" which is implemented by this patch as a WebKit option. Originally I had simply
fixed it in another webkit patch (24722) but that was rejected under the grounds that some people
wanted this behavior as an option, although there was agreement (see comments there) that the
springloaded behavior was desirable.

> It looks like it adds an option to toggle between "always do sticky autoscroll"
> (the old behavior) and "always do spring-loaded autoscroll".
Correct.

> If this is in fact what it does, no client wants this.  Certainly what Chromium
> wants (and what I suspect Safari will want) is more what Loren's patch aimed
> for: spring-loaded autoscroll on drag, sticky otherwise.
That is wrong. See below.

> Can someone explain what benefit this patch has?  If not, I intend to revert it
> after a reasonable amount of time has passed.

The behavior for prior to this patch was problematic because when one accidentally middle-clicks
on something the user inadvertently enters autoscroll mode and the page often moves unpredicatbly.
This occurs a lot and is extremely annoying to some users. You can read the long threads attached
to Chromium issues 5759 and 5162. For example, comment#9 is typical:

"I was so happy Chrome didn't have this feature for a long time (or at least it never 
worked on my PC I just didn't know about it) and then I updated to the latest chrome 
last week (around 5/23/09) and now I am ALWAYS accidentally middle clicking and the 
webpage FLYS  AROUND as I try to regain control again."

The code here gives an option that allows WebKit to expose either this behavior or the one
described here which is a more efficient way of scrolling and does not exhibit the problem
reported by our users:
- Spring loaded autoscroll means that users come of autoscroll automatically upon mouse up.
So they don't lose the position on their page.
- In sticky mode, it takes 5 actions (down, up, drag, down, up) to move the page and be
able to interact with it. In sringloaded mode, it takes 3 actions "down, drag, up".

Now, I do see that a number of very similar bugs (but not identical) were filed in both
WebKit and Chromium which may have led to some confusion but clearly show that users are
having issues here. WebKit now has the luxury of providing both types of autoscroll and
also address the problem of accidentally entering autoscroll mode.

[Itai]

(In reply to comment #34)
> (In reply to comment #33)
> > someone should probably resolve sine it has been implemented.
> > It is the responsibility of the browsers to use the preference as needed.
> 
> Itai, your patch should have been attached to bug 24791, not this bug. Bug
> 24791 was about implementing a preference, and has been marked RESOLVED/FIXED
> since your patch landed.
> 
> This bug is about a different issue: performing the sequence "middle down,
> middle up, move" should leave you in pan-to-scroll mode (as it does today), but
> performing the sequence "middle down, move, middle up" should stop the
> pan-to-scroll (this is the "springloaded" behavior).
> 
> The preference is an entirely separate issue (which is why it's represented by
> a different bug, even though your patches ended up mistakenly attached to this
> one).
> 
Adam, I hope the comment I just wrote clears some confusion about this but neither
issue covers both cases as the patch which I submitted does. Bug 24791 is about
disabling this entirely while this bug is about have both behaviors. The given
gives both behaviors as requested by this issue. It does so in a manner that
should greatly reduce the reasons for requesting patch 24791.

[Peter Kasting]

(In reply to comment #35)
> The title of this one is "Middle-click panning
> should be springloaded
> while dragging" which is implemented by this patch as a WebKit option.

No, according to you (and my reading of the patch code), this implements "Middle-click panning should be springloaded always" as an option.

> > If this is in fact what it does, no client wants this.  Certainly what Chromium
> > wants (and what I suspect Safari will want) is more what Loren's patch aimed
> > for: spring-loaded autoscroll on drag, sticky otherwise.
> That is wrong. See below.

Your pasted motivation below is a user comment on a Chromium bug.  This comment implies nothing about what Chromium as a WebKit client is actually going to do.  What Chromium wants, as explicitly specified by the UI design team, is to springload autoscroll on middle-click drag, and to have sticky autoscroll on middle click, with no options.  This is also exactly what this bug requests, and what Loren's original patch was aiming for.

Furthermore, even if the comment was somehow representative of Chromium's design position, it requests the ability to toggle off autoscrolling entirely, a la bug 24791, not the option you have implemented here.

I am now convinced that the patch as landed is undesirable to everyone and should be backed out entirely.  It never belonged on this bug to begin with.

Itai, next time you want to "fix" some Chromium bugs upstream, clear your solutions with the Chromium UI team *before* implementing them, not after or, in this case, never.  See the email thread Ben Goodger started recently on chromium-dev@.

[Peter Kasting]

Reverted r44276 in r44371.  The desired behavior for this bug (and for Chromium downstream) is something more like what Loren's patch series was going for.

[Jon Honeycutt]

Sorry, I should've paid closer attention when reviewing this patch. Thanks, Peter, for rolling this change out.

[Loren Segal]

(In reply to comment #38)
> Reverted r44276 in r44371.  The desired behavior for this bug (and for Chromium
> downstream) is something more like what Loren's patch series was going for.
> 

So what exactly is the status of this report? There's still no explanation as to why my patch was dropped. Is there anyone available to re-review this issue?

[Peter Kasting]

(In reply to comment #40)
> So what exactly is the status of this report? There's still no explanation as
> to why my patch was dropped.

I assume aroben r-ed it due to his notes in comment 8.  I haven't tested to see if I reproduce them, if the problem is pre-existing, etc.  Tracking those issues down is probably the correct next step.

[Brian Weinstein]

Created attachment 33404 [details]
Pan Scrolling

[Jon Honeycutt]

Comment on attachment 33404 [details]
Pan Scrolling

> --- a/WebCore/page/EventHandler.cpp
> +++ b/WebCore/page/EventHandler.cpp
> @@ -141,6 +141,8 @@ EventHandler::EventHandler(Frame* frame)
>      , m_mouseDownWasSingleClickInSelection(false)
>      , m_beganSelectingText(false)
>      , m_panScrollInProgress(false)
> +    , m_panScrollButtonPressed(false)
> +    , m_pressedPanScrollInProgress(false)

I think we should stick with the term "spring-loaded pan scroll" rather than "pressed pan scroll".

> @@ -676,6 +678,9 @@ void EventHandler::setPanScrollCursor()
>      bool north = m_panScrollStartPos.y() > (m_currentMousePosition.y() + ScrollView::noPanScrollRadius);
>      bool south = m_panScrollStartPos.y() < (m_currentMousePosition.y() - ScrollView::noPanScrollRadius);
>           
> +    if ((east || west || north || south) && m_panScrollButtonPressed)
> +        m_pressedPanScrollInProgress = true;
> +

It's strange to add this logic to setPanScrollCursor(). We should either move this logic out or rename the function.

r=me, but please fix the above issues.

[Brian Weinstein]

Landed in revision 46377.