Bug 29431

Summary: [Qt] REGRESSION:(r50665) QWebFrame::setScrollBarPolicy(Qt::Vertical,Qt::ScrollBarAlwaysOff) has no effect.
Product: WebKit Reporter: Tor Arne Vestbø <vestbo>
Component: WebKit QtAssignee: Antonio Gomes <tonikitoo>
Status: CLOSED FIXED    
Severity: Major CC: alice.barraclough, benjamin, eric, gns, hausmann, hyatt, ismail, jesus, jturcotte, kenneth, luiz, mark, tonikitoo, webkit.review.bot, xan.lopez
Priority: P1 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: Other   
OS: All   
Bug Depends on:    
Bug Blocks: 35784    
Attachments:
Description Flags
WIP - it works but is not to be reviewed or landed
none
patch_1 - v1 - adds lock scrollbars concept to ScrollView
hyatt: review-, tonikitoo: commit-queue-
patch 1 - v2 - addressed feedback from hyatt on IRC
none
patch_1 - v2.1 - same as "patch_1 - v2" rebased with ToT
tonikitoo: commit-queue-
(committed r57278) patch_2 - v1 - add stubs for Mac, Gtk, Wx and Win DRTs to implement this method
none
patch_1 - v2.2 - attempt to build on Mac (same as "patch_1 - v2.1")
hyatt: review-, tonikitoo: commit-queue-
patch_1 - v3 - addressed discussion with dhyatt on IRC
tonikitoo: commit-queue-
patch_1 - v3.1 - addressed discussion with dhyatt on IRC + rebased to ToT
tonikitoo: commit-queue-
(committed r57277) patch_1 - v3.2 - addressed discussion with dhyatt on IRC + rebased to ToT + builds in gtk none

Description Tor Arne Vestbø 2009-09-18 07:45:20 PDT
This bug report originated from issue QTBUG-3366
<http://bugreports.qt.nokia.com/browse/QTBUG-3366>

--- Description ---

QWebFrame::setScrollBarPolicy(Qt::Vertical,Qt::ScrollBarAlwaysOff) has no effect, and setScrollBarPolicy(Qt::Horizontal,Qt::ScrollBarAlwaysOff) has similar issues.

Launch the browser with the patch below applied. Notice that vertical scrollbars are rendered during load, but not once load has completed, but appear again upon resizing the window. The vertical one is always there.


diff --git a/demos/browser/webview.cpp b/demos/browser/webview.cpp
index 6ba41ee..c85726b 100644
--- a/demos/browser/webview.cpp
+++ b/demos/browser/webview.cpp
@@ -170,6 +170,8 @@ WebView::WebView(QWidget* parent)
             this, SLOT(downloadRequested(const QNetworkRequest &)));
     page()->setForwardUnsupportedContent(true);

+    page()->mainFrame()->setScrollBarPolicy(Qt::Horizontal,Qt::ScrollBarAlwaysOff);
+    page()->mainFrame()->setScrollBarPolicy(Qt::Vertical,Qt::ScrollBarAlwaysOff);
 }

 void WebView::contextMenuEvent(QContextMenuEvent *event)
@@ -222,6 +224,9 @@ void WebView::loadFinished()
                    << "Url:" << url();
     }
     m_progress = 0;
+
+    page()->mainFrame()->setScrollBarPolicy(Qt::Horizontal,Qt::ScrollBarAlwaysOff);
+    page()->mainFrame()->setScrollBarPolicy(Qt::Vertical,Qt::ScrollBarAlwaysOff);
 }

 void WebView::loadUrl(const QUrl &url)
fenglich@englich:qt/4.5:~/dev/qt-45/demos/bro
Comment 1 Ismail Donmez 2009-11-18 04:12:57 PST
I can reproduce this with latest qtwebkit 4.6 snapshot.
Comment 2 Antonio Gomes 2010-01-19 11:15:57 PST
raising the importance, since it regresses a previously working API-related code.

I can investigate ...
Comment 3 Antonio Gomes 2010-03-26 09:30:27 PDT
(In reply to comment #2)
> raising the importance, since it regresses a previously working API-related
> code.
> 
> I can investigate ...

just found the guilt commit (by git bisect'ing 480 commits) ... what broke it was b0444cef4e8bb4265a2b6d67e1f7d891680ec082
Comment 4 Kenneth Rohde Christiansen 2010-03-26 09:41:21 PDT
Please write revision instead. It seems to be r50665
Comment 5 Antonio Gomes 2010-03-26 13:32:20 PDT
void FrameView::layout(bool allowSubtree)
{
    (...)
    if (!m_canHaveScrollbars) {
        hMode = ScrollbarAlwaysOff;
        vMode = ScrollbarAlwaysOff;
    } else {
        scrollbarModes(hMode, vMode);
        if (hMode == ScrollbarAlwaysOff)
            hMode = ScrollbarAuto;
        if (vMode == ScrollbarAlwaysOff)
            vMode = ScrollbarAuto;
    }


... this else block broke us
Comment 6 Antonio Gomes 2010-03-30 12:56:06 PDT
Created attachment 52071 [details]
WIP - it works but is not to be reviewed or landed
Comment 7 Antonio Gomes 2010-04-01 13:17:00 PDT
adding alice (Apple) and mark (chromium) who were the reporters of the regressions that ended up causing this problem, as well as hyatt the reviewer of one of them.
Comment 8 Antonio Gomes 2010-04-01 13:21:54 PDT
Created attachment 52325 [details]
patch_1 - v1 - adds lock scrollbars concept to ScrollView

Patch introduces a lock scrollbars concept to ScrollView, as in WebDynamicScrollBarsView.mm/h on WebKit/mac. It is needed because in QtWebKit, we have Api for setting both vertical and horizontal scrollbars on/off/auto. When it is set to off, for example, it should remain as such, unless unset.


@hyatt, do you think this approach is feasible?
Comment 9 Eric Seidel (no email) 2010-04-01 13:30:30 PDT
Attachment 52325 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1572150
Comment 10 Dave Hyatt 2010-04-01 13:41:43 PDT
Comment on attachment 52325 [details]
patch_1 - v1 - adds lock scrollbars concept to ScrollView

I would not alter any of the mode-setting functions.  Just add the lock functions separately.  The mode-setting functions can of course check the locks, but I don't think you should mix mode setting and locking in the same functions.
Comment 11 Antonio Gomes 2010-04-04 20:11:07 PDT
Created attachment 52517 [details]
patch 1 - v2 - addressed feedback from hyatt on IRC

In IRC, it was agreed w/ hyatt to remove the optional 'lock' parameter from ScrollView::setScrollingModes, since both set{Horizontal,Vertical}ScrollingMode methods have the 'lock' parameter, which is enough to fixing the issue. The former *has* honors the current lock state though.
 
It was also agreed Frame::createView could get two additional parameters for vertical scrollbar lock and horizontal scrollbar lock respectively, to persist the current lock state cross page loads.

patch 2 addresses these.
Comment 12 Antonio Gomes 2010-04-05 14:15:08 PDT
Created attachment 52567 [details]
patch_1 - v2.1 - same as "patch_1 - v2" rebased with ToT
Comment 13 Antonio Gomes 2010-04-05 14:16:12 PDT
Created attachment 52568 [details]
(committed r57278) patch_2 - v1 - add stubs for Mac, Gtk, Wx and Win DRTs to implement this method
Comment 14 Antonio Gomes 2010-04-05 14:16:57 PDT
@hyatt, could you help me w/ this two (complementary) patches?
Comment 15 Antonio Gomes 2010-04-06 08:33:35 PDT
Created attachment 52641 [details]
patch_1 - v2.2 - attempt to build on Mac (same as "patch_1 - v2.1")
Comment 16 Dave Hyatt 2010-04-06 12:45:40 PDT
Comment on attachment 52641 [details]
patch_1 - v2.2 - attempt to build on Mac (same as "patch_1 - v2.1")

(1) For the createView function, having two calls to set scrollbar modes is problematic when each one results in an updateScrollers call underneath, e.g., on Mac.  You could just do:

frameView->setHorizontalLock(horizontalLock);
frameView->setVerticalLock(verticalLock);
frameView->setScrollbarModes(horizontalScrollbarMode, verticalScrollbarMode);

(2) Dump the lock argument from setHorizontalScrollbarMode and setVerticalScrollbarMode. It's just inconsistent with setScrollbarModes, and you've provided locking functions anyway (setHorizontalLock and setVerticalLock).

(3) in qwebframe.cpp just do the lock call before setting the mode, e.g.,

d->frame->view()->setHorizontalLock(policy != Qt::ScrollBarAsNeeded);
d->frame->view()->setHorizontalScrollbarMode((ScrollbarMode)policy);

It's slightly more verbose, but I think it's nice to separate.
Comment 17 Antonio Gomes 2010-04-06 13:25:19 PDT
(In reply to comment #16)
> (From update of attachment 52641 [details])
> (1) For the createView function, having two calls to set scrollbar modes is
> problematic when each one results in an updateScrollers call underneath, e.g.,
> on Mac.  You could just do:
> 
> frameView->setHorizontalLock(horizontalLock);
> frameView->setVerticalLock(verticalLock);
> frameView->setScrollbarModes(horizontalScrollbarMode, verticalScrollbarMode);
> 
> (2) Dump the lock argument from setHorizontalScrollbarMode and
> setVerticalScrollbarMode. It's just inconsistent with setScrollbarModes, and
> you've provided locking functions anyway (setHorizontalLock and
> setVerticalLock).
> 
> (3) in qwebframe.cpp just do the lock call before setting the mode, e.g.,
> 
> d->frame->view()->setHorizontalLock(policy != Qt::ScrollBarAsNeeded);
> d->frame->view()->setHorizontalScrollbarMode((ScrollbarMode)policy);
> 
> It's slightly more verbose, but I think it's nice to separate.

... and discussion continued on IRC:

<tonikitoo> dhyatt,  you suggested doing:
<tonikitoo> "setHorizontalLock(hLock); setVerticalLock(vLock); setScrollbarModes(...)"
<tonikitoo> but if setScrollbarModes honors the lock, it wont change to the policy you are setting, because it is already locked.
<dhyatt> put them after then?
<dhyatt> setScrollbarModes(....); setHorizontalLock(hLock); setVerticalLock(vLock); ?
<tonikitoo> dhyatt, after can be also tricky, because a FrameView::layout() is called during setScrollbarModes, and layout() can change it 
<tonikitoo> I've tested these possibilities
<tonikitoo> dhyatt, suppose we are calling lock after, then  setScrollbarModes which calls updateScrollbars, and endup calling FrameView::layout , where it is scrollbar mode/policy unset, and then you lock, it would not work =/
<dhyatt> tonikitoo: yeah true
<dhyatt> i guess if you have to lock at the same time you could leave it the way it is
<dhyatt> ok, i guess the lock has to be part of the cal llike you did it then
<dhyatt> setScrollbarModes should take the locks too then
<dhyatt> for consistency with the setHorizontal and setVertical methods and so you can do the one call in createView

that is the way I'll do it
Comment 18 Antonio Gomes 2010-04-07 08:12:47 PDT
Created attachment 52736 [details]
patch_1 - v3 - addressed discussion with dhyatt on IRC

As accorded on IRC, this patch:

* adds additional lock parameters to setScrollbarModes, setVerticalScrollbarMode and setHorizontalScrollbarMode of ScrollView;
* make the latter two just call the former passing the appropriated parameters, as originally;
* adds two optional lock parameters to Frame::createView, for locking horizontal and vertical scrollbars, respectively.
* adds a layout test and a qt API unit test
* make the adjusts needed in FrameLoaderClientQt, to use the new createView parameters.
Comment 19 Antonio Gomes 2010-04-07 08:26:19 PDT
Created attachment 52737 [details]
patch_1 - v3.1 - addressed discussion with dhyatt on IRC + rebased to ToT

see comment #18
Comment 20 WebKit Review Bot 2010-04-07 08:32:38 PDT
Attachment 52737 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/ScrollView.h:88:  More than one command on the same line  [whitespace/newline] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/tests/qwebframe/tst_qwebframe.cpp"
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 WebKit Review Bot 2010-04-07 09:22:33 PDT
Attachment 52737 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1654226
Comment 22 Antonio Gomes 2010-04-07 10:13:11 PDT
Created attachment 52745 [details]
(committed r57277) patch_1 - v3.2 - addressed discussion with dhyatt on IRC + rebased to ToT + builds in gtk

see comment #18 for summary

@dhyatt, could you have a look?
Comment 23 WebKit Review Bot 2010-04-07 10:22:01 PDT
Attachment 52745 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/ScrollView.h:88:  More than one command on the same line  [whitespace/newline] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/tests/qwebframe/tst_qwebframe.cpp"
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Dave Hyatt 2010-04-07 14:05:54 PDT
Comment on attachment 52745 [details]
(committed r57277) patch_1 - v3.2 - addressed discussion with dhyatt on IRC + rebased to ToT + builds in gtk

r=me
Comment 25 Antonio Gomes 2010-04-08 08:17:28 PDT
Comment on attachment 52745 [details]
(committed r57277) patch_1 - v3.2 - addressed discussion with dhyatt on IRC + rebased to ToT + builds in gtk

thank you dhyatt!
Comment 26 Antonio Gomes 2010-04-08 08:18:26 PDT
Comment on attachment 52568 [details]
(committed r57278) patch_2 - v1 - add stubs for Mac, Gtk, Wx and Win DRTs to implement this method

thank you tronical/simon/hausmann!
Comment 27 Simon Hausmann 2010-04-09 00:37:33 PDT
<cherry-pick-for-backport: 57277>
Comment 28 Simon Hausmann 2010-04-09 00:38:02 PDT
<cherry-pick-for-backport: 57278>
Comment 29 Simon Hausmann 2010-04-09 00:41:22 PDT
Revision r57277 cherry-picked into qtwebkit-2.0 with commit dd4a747cf19884f3c1810e32f15abc5f0151ef04Revision r57278 cherry-picked into qtwebkit-2.0 with commit 9cf99c055c470bfc55c21753d38853a6f2d334b7