Bug 75006 - [Qt] Scrolling in touch mode
Summary: [Qt] Scrolling in touch mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on: 84243
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-21 05:21 PST by Allan Sandfeld Jensen
Modified: 2012-05-15 08:07 PDT (History)
7 users (show)

See Also:


Attachments
Patch (6.26 KB, patch)
2012-01-03 04:06 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (6.79 KB, patch)
2012-03-12 03:42 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (10.58 KB, patch)
2012-05-14 07:46 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch for landing (10.60 KB, patch)
2012-05-15 05:57 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2011-12-21 05:21:33 PST
This bug is created to track several related problem with scrolling in Qt Webkit2 touch-interface mode.

When web elements are set as scrollable with overflow:auto/scroll, they are currently neither displayed as scrollable, nor actually scrollable from the touch interface in Qt Webkit2. There is a patch 
https://gitorious.org/+qtwebkit-webkit2-dev/webkit/qtwebkit-webkit2-dev/commit/6c2e96afc4b384bfcb40008abb4eee6e3ad8ac81
that enables twofinger scrolling in these elements, but still doesn't indicate the elements are scrollable, nor make them scrollable by mouse or keyboard.

A related problem is that the even the root web-element/page is not scrollable by keyboard in WebKit2 touch-mode. 

The central problem is that traditional desktop scrolling is handled by WebCore, while touch-scrolling is handled in the Qt Platform code of the UI-process. WebCore ignores all the scroll-events in touch mode because scrollbars are absent which in WebCore indicates non-scrollable elements (overflow: hidden/visible), and the Qt Platform code does not yet duplicate all WebCore scrolling.

There needs to be a closer cooperation between the two scrolling implementations, or alternatively the touch interface implementation needs to be greatly expanded.
Comment 1 Kenneth Rohde Christiansen 2011-12-21 05:30:57 PST
I cannot agree more. Something related it looking into using a separate AC layer for div overflow and use our own interaction/physics engine with it.

I believe that the latest iOS WebKit code dump have patches related to this. RIM have also done something similar and it would be possible to talk to Antonio about that (cc'ing him).
Comment 2 Allan Sandfeld Jensen 2012-01-03 04:06:11 PST
Created attachment 120931 [details]
Patch
Comment 3 Kenneth Rohde Christiansen 2012-01-03 04:12:58 PST
Comment on attachment 120931 [details]
Patch

Thinking a bit more about this, I am not sure this is the best approach.

It would be better to move the smooth scrolling to the UI side for our case and check whether an event listener was added for the wheel event/page up/page down etc. And as long as that is not added we should just scroll on the ui side immediately. If one of these events are being listened to, we need to send the event to WebCore and if it is not prevented act on it on the UI side.

Doing it this way would make it very similar to how we were handling touch events in the N9 browser.
Comment 4 Allan Sandfeld Jensen 2012-01-03 05:44:17 PST
Just to clarify. There is no smooth scrolling in the this patch. It is only keyboard and mouse-wheel scrolling. 

It works by letting the events be handled by webcore, but having webcore request the final scrolling if any from the UI process.
Comment 5 Antonio Gomes 2012-01-03 06:23:44 PST
(In reply to comment #4)
> Just to clarify. There is no smooth scrolling in the this patch. It is only keyboard and mouse-wheel scrolling. 
> 
> It works by letting the events be handled by webcore, but having webcore request the final scrolling if any from the UI process.

that sounds similar to what we did, though not so much. I will take a deeper look at the patch soon.
Comment 6 Allan Sandfeld Jensen 2012-01-09 06:14:17 PST
Comment on attachment 120931 [details]
Patch

Currently being revised. Removing review.
Comment 7 Allan Sandfeld Jensen 2012-03-12 03:42:52 PDT
Created attachment 131312 [details]
Patch
Comment 8 Kenneth Rohde Christiansen 2012-03-12 07:30:18 PDT
Comment on attachment 131312 [details]
Patch

Doesn't this mean that it will scroll in different steps depending on your zoom level?
Comment 9 Allan Sandfeld Jensen 2012-03-12 07:48:59 PDT
(In reply to comment #8)
> (From update of attachment 131312 [details])
> Doesn't this mean that it will scroll in different steps depending on your zoom level?

Yes, it will always try to scroll 3 lines, which can be a small step on fully zoomed out page. But keep in mind that this is for a special case of getting actual wheel-mouse events on a touch-based browser (desktop/touch hybrid). It could be mouse over a 'overflow:scroll' element that is only a part of a large page. Should this sub-element scroll faster because the page is zoomed out?

Additionally I think some of the code (even the DOM wheel-event spec) demands the mouse-wheel delta is in multipla of 120. So zooming the scroll-delta might not be that doable without breaking some assumptions and the DOM event spec.
Comment 10 Kenneth Rohde Christiansen 2012-03-14 05:31:13 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 131312 [details] [details])
> > Doesn't this mean that it will scroll in different steps depending on your zoom level?
> 
> Yes, it will always try to scroll 3 lines, which can be a small step on fully zoomed out page. But keep in mind that this is for a special case of getting actual wheel-mouse events on a touch-based browser (desktop/touch hybrid). It could be mouse over a 'overflow:scroll' element that is only a part of a large page. Should this sub-element scroll faster because the page is zoomed out?
> 
> Additionally I think some of the code (even the DOM wheel-event spec) demands the mouse-wheel delta is in multipla of 120. So zooming the scroll-delta might not be that doable without breaking some assumptions and the DOM event spec.

So does it actually scroll larger distances for larger fonts?

The spec could be considered to deal with a scale of 1.0 as this probably wasn't considered when it was written. Scrolling quite slowly when zoomed out, seems quite weird to me.
Comment 11 Kenneth Rohde Christiansen 2012-03-14 05:32:09 PDT
Comment on attachment 131312 [details]
Patch

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

> Source/WebCore/platform/ScrollView.cpp:370
>      repaintFixedElementsAfterScrolling();
> +#if USE(TILED_BACKING_STORE)
> +    if (delegatesScrolling()) {
> +        hostWindow()->delegatedScrollRequested(IntPoint(newOffset));
> +        return;
> +    }
> +#endif
>      scrollContents(scrollDelta);

Is this part specific to this patch?
Comment 12 Allan Sandfeld Jensen 2012-03-14 08:40:03 PDT
(In reply to comment #11)
> (From update of attachment 131312 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131312&action=review
> 
> > Source/WebCore/platform/ScrollView.cpp:370
> >      repaintFixedElementsAfterScrolling();
> > +#if USE(TILED_BACKING_STORE)
> > +    if (delegatesScrolling()) {
> > +        hostWindow()->delegatedScrollRequested(IntPoint(newOffset));
> > +        return;
> > +    }
> > +#endif
> >      scrollContents(scrollDelta);
> 
> Is this part specific to this patch?

Yes, this is heart of the patch. It takes redirects scrolling done by webcore and requests it from the hostWindow if scrolling is delegated to the hostWindow.
Comment 13 Allan Sandfeld Jensen 2012-04-18 08:21:41 PDT
To support scaling wheel-events I first needed to fix several bugs in our support of fractional wheel-events. There is a patch for it in #84243, once in, the patch here can be updated.
Comment 14 Allan Sandfeld Jensen 2012-05-14 07:46:48 PDT
Created attachment 141727 [details]
Patch
Comment 15 Kenneth Rohde Christiansen 2012-05-15 05:39:47 PDT
Comment on attachment 141727 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1725
> +    bool visibleSizeChanged = false;

I think we should use visibleSizeDidChange as that seems more common today in common code
Comment 16 Allan Sandfeld Jensen 2012-05-15 05:57:26 PDT
Created attachment 141939 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-05-15 08:07:15 PDT
Comment on attachment 141939 [details]
Patch for landing

Clearing flags on attachment: 141939

Committed r117069: <http://trac.webkit.org/changeset/117069>
Comment 18 WebKit Review Bot 2012-05-15 08:07:22 PDT
All reviewed patches have been landed.  Closing bug.