Bug 53917

Summary: [Qt] QtTestBrowser - Horizontal scrollbar disappears on navigating pages using Back/Forward
Product: WebKit Reporter: Aparna Nandyal <aparna.nand>
Component: Tools / TestsAssignee: Aparna Nandyal <aparna.nand>
Status: RESOLVED FIXED    
Severity: Normal CC: aparna.nand, commit-queue, kling, menard, tonikitoo
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Screenshot of the browser without horizontal scrollbar
none
Patch for review
tonikitoo: review-, tonikitoo: commit-queue-
A simple html file to reproduce the problem
none
Patch for review
none
Patch for review
none
Patch with tests for review
none
Patch with tests for review none

Description Aparna Nandyal 2011-02-07 07:45:09 PST
Created attachment 81471 [details]
Screenshot of the browser without horizontal scrollbar

Steps to reproduce:
1. Launch QtTestBrowser and resize it so the horizontal scrollbars appear
2. Go to any url say, google.com (Page 1) and perform a search 
3. In the results page (page 2), the horizontal scrollbar should appear
4. Click a link on the results page and navigate to another page (Page 3)
5. Now using Back button go back to previous page.

Expected results:
It should take us back to Page 2 and the horizontal scroll bar should appear

Actual results:
Horizontal scroll bar does not appear, so content that can be viewed on the page is restricted.

Other information:
1. Reproducible on arora browser
2. On resizing the page (Page 2), the horizontal scrollbar appears
Comment 1 Aparna Nandyal 2011-02-07 12:32:29 PST
Analysis:
1. The problem is seen when the page is loaded from the cache.
2. In the code path for loading from cache - In 
FrameView::layout()
{
 ..
 if (hMode == ScrollbarAuto)
                    setHorizontalScrollbarMode(ScrollbarAlwaysOff); // This causes a horizontal scrollbar to disappear.

  setScrollbarModes(hMode, vMode);
  setScrollbarsSuppressed(false, true);

...
}

The horizontal scrollbar is removed and the conditions to display it again is hit only when Content sizes is updated. Since the content is being retrieved from cache, the ScrollView::setContentsSize() is never called and hence ScrollView::updateScrollbars() is not called to update the horizontal scrollbar.

There are 2 options to fix this:
1. Set the contentsSize to default so, when the layout is being done, the actual contents size will be different and this will invoke ScrollView::updateScrollbars.

2. In FrameView::layout, swap the lines of code 
setScrollbarsSuppressed(false, true);
setScrollbarModes(hMode, vMode);

The second option is probably not the way to go because the scroll bar has been deliberately removed and suppressed till the ScrollView::updateScrollbars decides to enable it.
Comment 2 Aparna Nandyal 2011-02-07 12:34:29 PST
Created attachment 81504 [details]
Patch for review

Patch to fix the missing horizontal scrollbar on doing a back
Comment 3 Antonio Gomes 2011-02-07 12:45:46 PST
Comment on attachment 81504 [details]
Patch for review

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

I would like to see a layout test for it, apart from a auto test

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3117
> +    QTest::qWait(2000);

do you really need to wait that long?
Comment 4 Aparna Nandyal 2011-02-09 11:27:27 PST
Created attachment 81839 [details]
A simple html file to reproduce the problem

A html file
1. Loads a page with horizontal scrollbar
2. Navigates away from it and comes back to same page.
3. With the bug, horizontal scrollbar disappears on coming back.
Comment 5 Antonio Gomes 2011-02-09 11:43:31 PST
LayoutTests/fast/history/back-forward-reset-after-error-handling.html can get you an idea of a nice layout test for it...
Comment 6 Aparna Nandyal 2011-02-10 03:16:10 PST
Created attachment 81946 [details]
Patch for review

Submitting it by adding layout test.
Comment 7 Antonio Gomes 2011-02-10 05:56:18 PST
Comment on attachment 81946 [details]
Patch for review

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

> LayoutTests/fast/overflow/horizontal-scroll-after-back.html:24
> +    window.setTimeout("verify()", 200);

Not sure if this is correct: when you navigate way from a page, async scripts of the 1st page are probably cancelled. I would suggest you using queueLoad, queueBack, and friends from LayoutTestController class.
Comment 8 Aparna Nandyal 2011-02-10 09:45:46 PST
Created attachment 81995 [details]
Patch for review

Removed the async call.
Comment 9 Antonio Gomes 2011-02-10 10:06:05 PST
Comment on attachment 81995 [details]
Patch for review

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

> LayoutTests/fast/overflow/horizontal-scroll-after-back.html:39
> +    // is happening as result of navigation back then we proceed.

// If this is not the first time pageshow and pageshow
// is happening as result of navigation back then we proceed. 

It reads strange.

> Source/WebCore/page/FrameView.cpp:864
> +                setContentsSize(IntSize());

since it is for the first layout, it looks safe, but I would like to double check why only Qt port seem affected by this?
Comment 10 Antonio Gomes 2011-02-23 21:06:38 PST
> 
> > Source/WebCore/page/FrameView.cpp:864
> > +                setContentsSize(IntSize());
> 
> since it is for the first layout, it looks safe, but I would like to double check why only Qt port seem affected by this?

Ping!?
Comment 11 Aparna Nandyal 2011-02-25 07:39:44 PST
This bug is no longer reproducible, I guess thanks to recent changes in ScrollView and FrameView. But do you think we should still add the tests to avoid future regressions?
Comment 12 Antonio Gomes 2011-02-25 08:30:22 PST
(In reply to comment #11)
> This bug is no longer reproducible, I guess thanks to recent changes in ScrollView and FrameView. But do you think we should still add the tests to avoid future regressions?

yes
Comment 13 Aparna Nandyal 2011-02-27 12:24:35 PST
Created attachment 83985 [details]
Patch with tests for review

Creating a patch with test cases
Comment 14 Antonio Gomes 2011-02-27 15:43:17 PST
Comment on attachment 83985 [details]
Patch with tests for review

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

> LayoutTests/fast/overflow/horizontal-scroll-after-back.html:52
> +<body onload='runTestStep()'>

Are you sure runTestStep runs in the second load of this page? iirc, since you enabled page cache, pageShow() event is triggered instead. Could you confirm?
Comment 15 Aparna Nandyal 2011-02-27 21:13:31 PST
Created attachment 84013 [details]
Patch with tests for review

Uploaded a different html file with no async call.
Comment 16 Aparna Nandyal 2011-02-27 21:15:34 PST
 
> Are you sure runTestStep runs in the second load of this page? iirc, since you enabled page cache, pageShow() event is triggered instead. Could you confirm?

Sorry for the rework. I had uploaded the wrong html file instead of the one without async call.
Comment 17 WebKit Commit Bot 2011-02-27 23:30:24 PST
Comment on attachment 84013 [details]
Patch with tests for review

Clearing flags on attachment: 84013

Committed r79844: <http://trac.webkit.org/changeset/79844>
Comment 18 WebKit Commit Bot 2011-02-27 23:30:30 PST
All reviewed patches have been landed.  Closing bug.