Bug 26695 – Windows 7: Dragging Scrollbar doesn't work properly on inner divs with scrollbars or Frames on Touch Screen

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brian Weinstein]

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

[Brian Weinstein]

<rdar://problem/7004204>

[Brian Weinstein]

Created attachment 31873 [details]
Inner Scrollbar Patch

Patch to fix grabbing of inner scrollbars on textareas, divs with overflow, and frames with scrollbars.

[Adam Roben (:aroben)]

Comment on attachment 31873 [details]
Inner Scrollbar Patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 45184)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,24 @@
> [phone]  Brian Weinstein  <[e-mail]>
> +
> +        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!

[Brian Weinstein]

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

[Brian Weinstein]

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.

[Brian Weinstein]

Comment on attachment 31957 [details]
Take 2 Panning Patch

Clearing Review Flag - Landed in 45304