Bug 26695 - Windows 7: Dragging Scrollbar doesn't work properly on inner divs with scrollbars or Frames on Touch Screen
Summary: Windows 7: Dragging Scrollbar doesn't work properly on inner divs with scroll...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P3 Normal
Assignee: Brian Weinstein
URL:
Keywords: InRadar, PlatformOnly
Depends on:
Blocks: 26486
  Show dependency treegraph
 
Reported: 2009-06-24 15:06 PDT by Brian Weinstein
Modified: 2009-07-02 20:58 PDT (History)
1 user (show)

See Also:


Attachments
Inner Scrollbar Patch (10.92 KB, patch)
2009-06-25 14:03 PDT, Brian Weinstein
aroben: review-
Details | Formatted Diff | Diff
Take 2 Panning Patch (16.20 KB, patch)
2009-06-26 15:38 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Weinstein 2009-06-24 15:06:15 PDT
6/24/09 2:55 PM Brian Weinstein:
* SUMMARY
Unable to grab a scrollbar on an inner frame or div - it is always thought of as a panning gesture.

* STEPS TO REPRODUCE
1. Go to www.tivofaq.com
2. Try to scroll by dragging the scrollbar with your finger

* RESULTS
Interpreted as a panning gesture instead of dragging of the scrollbar
Comment 1 Brian Weinstein 2009-06-24 15:06:55 PDT
<rdar://problem/7004204>
Comment 2 Brian Weinstein 2009-06-25 14:03:42 PDT
Created attachment 31873 [details]
Inner Scrollbar Patch

Patch to fix grabbing of inner scrollbars on textareas, divs with overflow, and frames with scrollbars.
Comment 3 Adam Roben (:aroben) 2009-06-25 14:19:50 PDT
Comment on attachment 31873 [details]
Inner Scrollbar Patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 45184)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,24 @@
> +2009-06-25  Brian Weinstein  <bweinstein@apple.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +        
> +        https://bugs.webkit.org/show_bug.cgi?id=26695
> +        
> +        This patch adds the ability to get have the Scrollbar variable set
> +        in EventHandler::hitTestResultAtPoint, and if the mouse is over a
> +        scrollbar, the m_scrollbar variable will be set.

I'm not sure how the independent clauses of this sentence are different

> -HitTestResult EventHandler::hitTestResultAtPoint(const IntPoint& point, bool allowShadowContent, bool ignoreClipping)
> +HitTestResult EventHandler::hitTestResultAtPoint(const IntPoint& point, bool allowShadowContent, bool ignoreClipping, bool shouldSetScrollbar)

We're trying to avoid adding boolean parameters, because they are not very clear (when reading code like "hitTestResultAtPoint(point, true, false, true)", what do the booleans mean?). One strategy to replace a boolean parameter is to use a two-value enum, like this:

enum HitTestScrollbars { ShouldHitTestScrollbars, DontHitTestScrollbars };
HitTestResult hitTestResultAtPoint(const IntPoint&, bool allowShadowContent, bool ignoreClipping, HitTestScrollbars);

> +        if (shouldSetScrollbar) {
> +            PlatformMouseEvent mouseEvent(point, point, NoButton, MouseEventPressed, 0, false, false, false, false, 0);
> +            Scrollbar* eventScrollbar = view->scrollbarUnderMouse(mouseEvent);
> +            if (eventScrollbar)
> +                result.setScrollbar(eventScrollbar);
> +        }

Can we change scrollbarUnderMouse to take an IntPoint instead of a PlatformMouseEvent? If we do that, we might want to change its name to something like scrollbarAtPoint or scrollbarAtWindowPoint.

> +++ WebCore/platform/PlatformWheelEvent.h	(working copy)
> @@ -27,6 +27,8 @@
>  #define PlatformWheelEvent_h
>  
>  #include "IntPoint.h"
> +#include "FloatPoint.h"
> +#include "FloatSize.h"

You don't need these #includes here. You can just declare the classes.


> +++ WebCore/platform/win/WheelEventWin.cpp	(working copy)
> @@ -23,6 +23,8 @@
>   * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
>   */
>  
> +#include "FloatPoint.h"
> +#include "FloatSize.h"
>  #include "PlatformWheelEvent.h"
>  
>  #include <windows.h>

The #includes should be formatted like this:

#include "config.h"
#include "PlatformWheelEvent.h"

#include "FloatPoint.h"
#include "FloatSize.h"
#include <windows.h>

(It's a mistake that config.h wasn't listed before!)

> +        ScrollView* view = coreFrame->view();
> +        if (!view) {
> +            CloseGestureInfoHandlePtr()(gestureHandle);
> +            return false;
> +        }
> +        Scrollbar* vertScrollbar = view->verticalScrollbar();
> +        if (!vertScrollbar) {
> +            CloseGestureInfoHandlePtr()(gestureHandle);
> +            return true;
> +        }

Why "return false" in one case and "return true" in another?

I think ideally this change would be done as 3 patches:

1) Change the parameters to the PlatformWheelEvent constructor
2) Change the parameter to scrollbarUnderMouse (and maybe change its name, too)
3) Everything else

r- so that we can at least get the style issues fixed. But please consider splitting up the patch, too!
Comment 4 Brian Weinstein 2009-06-26 10:34:53 PDT
> > +        ScrollView* view = coreFrame->view();
> > +        if (!view) {
> > +            CloseGestureInfoHandlePtr()(gestureHandle);
> > +            return false;
> > +        }
> > +        Scrollbar* vertScrollbar = view->verticalScrollbar();
> > +        if (!vertScrollbar) {
> > +            CloseGestureInfoHandlePtr()(gestureHandle);
> > +            return true;
> > +        }
> 
> Why "return false" in one case and "return true" in another?

This is because if there is no ScrollView, then I feel like the call to ::gesture() has failed, however, if there is no Scrollbar, then the call to ::gesture() has succeeded and just isn't necessary. That is why I return true if there is no scrollbar, but false if there is no ScrollView.
Comment 5 Brian Weinstein 2009-06-26 15:38:19 PDT
Created attachment 31957 [details]
Take 2 Panning Patch

I was struggling to get this split into multiple patches, as the same section in the diff needed to go in more than 1 patch, so I left it in one for now, but if it's a dealbreaker to review, let me know.
Comment 6 Brian Weinstein 2009-06-29 10:46:06 PDT
Comment on attachment 31957 [details]
Take 2 Panning Patch

Clearing Review Flag - Landed in 45304