Bug 90550 - Wheel events on a page with frames are not handled in fixed layout
Summary: Wheel events on a page with frames are not handled in fixed layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Normal
Assignee: Andras Becsi
URL: http://www.quackit.com/html/templates...
Keywords: Qt
Depends on: 90547
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-04 07:51 PDT by Andras Becsi
Modified: 2012-07-30 07:23 PDT (History)
4 users (show)

See Also:


Attachments
proposed patch (2.57 KB, patch)
2012-07-04 08:04 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
Patch (2.78 KB, patch)
2012-07-25 08:30 PDT, Allan Sandfeld Jensen
tonikitoo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andras Becsi 2012-07-04 07:51:13 PDT
Since r117069 we also use ScrollView::updateScrollbars() if scroll is delegated (invisible scrollbars). This results in erroneous behavior in fixed layout mode on pages which use frames.
This is due to fixed layout enabling frame flattening in which case updateScrollbars disables the scrollbars on the page since the document size is equal to the frame size.

This is reproducible with MiniBrowser on the given URL by double tapping to zoom in and then trying to scroll.
Comment 1 Andras Becsi 2012-07-04 08:04:09 PDT
Created attachment 150797 [details]
proposed patch
Comment 2 Andras Becsi 2012-07-04 08:13:07 PDT
Comment on attachment 150797 [details]
proposed patch

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

> Source/WebCore/ChangeLog:17
> +        Need to figure out a way to test this, probably with
> +        a QML API test since WTR does not use frame flattening.

Turns out that there is testRunner.setFrameFlatteningEnabled(true), and there are layout tests for this. Need to check the Skipped list as well.
Comment 3 Andras Becsi 2012-07-04 08:16:03 PDT
This patch in combination with the patch of bug 90547 also fixes the scrolling in a simple QML WebView loading a simple local html.
Comment 4 Andras Becsi 2012-07-09 03:47:28 PDT
Comment on attachment 150797 [details]
proposed patch

Even though the patch fixes the scrolling issue it does not seem to be the right solution. I'll revisit this later. Removing patch from review queue.
Comment 5 Allan Sandfeld Jensen 2012-07-25 07:55:34 PDT
Comment on attachment 150797 [details]
proposed patch

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

> Source/WebCore/platform/ScrollView.cpp:509
>          IntSize frameSize = frameRect().size();
> +        bool usesFixedLayout = useFixedLayout();
>  
>          if (hScroll == ScrollbarAuto) {
>              newHasHorizontalScrollbar = docSize.width() > visibleWidth();
> -            if (newHasHorizontalScrollbar && !m_updateScrollbarsPass && docSize.width() <= frameSize.width() && docSize.height() <= frameSize.height())
> +            if (newHasHorizontalScrollbar && !m_updateScrollbarsPass && !usesFixedLayout && docSize.width() <= frameSize.width() && docSize.height() <= frameSize.height())

I think the real problem here is the use of frameRect().size() since with fixedLayout the frameRect is always the size of the document. It should use visibleContentRect(true) instead.
Comment 6 Allan Sandfeld Jensen 2012-07-25 08:30:40 PDT
Created attachment 154364 [details]
Patch

A quick alternative patch based on my observersion. This solves most of the problems, though some frames does not create the invisible scroll-bars until the page has been manipulated in some way. Though that is probably a separate bug with a missing updateScrollbar() call somewhere.
Comment 7 Antonio Gomes 2012-07-25 10:43:21 PDT
Comment on attachment 154364 [details]
Patch

why is this Qt specific?
Comment 8 Allan Sandfeld Jensen 2012-07-25 11:09:45 PDT
(In reply to comment #7)
> (From update of attachment 154364 [details])
> why is this Qt specific?

I guess it isn't as such, it is only specific to the way Qt uses FrameView, anyone using it the same way would have the same problem.
Comment 9 Allan Sandfeld Jensen 2012-07-30 05:46:20 PDT
But besides the commit title, is there any other comments to the solution, or can it go in?
Comment 10 Antonio Gomes 2012-07-30 06:13:33 PDT
By any chance, do you set fixedLayout for non-mainframe ScrollViews?
Comment 11 Allan Sandfeld Jensen 2012-07-30 06:30:32 PDT
(In reply to comment #10)
> By any chance, do you set fixedLayout for non-mainframe ScrollViews?

I don't think so, not unless it is a part of frame-flattening (I haven't checked that code in that much detail yet).
Comment 12 Allan Sandfeld Jensen 2012-07-30 07:23:32 PDT
Committed r124024: <http://trac.webkit.org/changeset/124024>