[Qt] QtTestBrowser - Horizontal scrollbar disappears on navigating pages using Back/Forward

[Aparna Nandyal]

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

[Aparna Nandyal]

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.

[Aparna Nandyal]

Created attachment 81504 [details]
Patch for review

Patch to fix the missing horizontal scrollbar on doing a back

[Antonio Gomes]

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?

[Aparna Nandyal]

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.

[Antonio Gomes]

LayoutTests/fast/history/back-forward-reset-after-error-handling.html can get you an idea of a nice layout test for it...

[Aparna Nandyal]

Created attachment 81946 [details]
Patch for review

Submitting it by adding layout test.

[Antonio Gomes]

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.

[Aparna Nandyal]

Created attachment 81995 [details]
Patch for review

Removed the async call.

[Antonio Gomes]

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?

[Antonio Gomes]

> 
> > 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!?

[Aparna Nandyal]

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?

[Antonio Gomes]

(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

[Aparna Nandyal]

Created attachment 83985 [details]
Patch with tests for review

Creating a patch with test cases

[Antonio Gomes]

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?

[Aparna Nandyal]

Created attachment 84013 [details]
Patch with tests for review

Uploaded a different html file with no async call.

[Aparna Nandyal]

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

[WebKit Commit Bot]

Comment on attachment 84013 [details]
Patch with tests for review

Clearing flags on attachment: 84013

Committed r79844: <http://trac.webkit.org/changeset/79844>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.