Bug 29431

Summary:

[Qt] REGRESSION:(r50665) QWebFrame::setScrollBarPolicy(Qt::Vertical,Qt::ScrollBarAlwaysOff) has no effect.

Product:

WebKit

Reporter:

Tor Arne Vestbø <vestbo>

Component:

WebKit Qt

Assignee:

Antonio Gomes <tonikitoo>

Status:

CLOSED FIXED

Severity:

Major

CC:

alice.barraclough, benjamin, eric, gustavo, hausmann, hyatt, ismail, jesus, jturcotte, kenneth, luiz, mark, tonikitoo, webkit.review.bot, xan.lopez

Priority:

Keywords:

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

Tor Arne Vestbø

Reported 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

Attachments
WIP - it works but is not to be reviewed or landed (6.28 KB, patch) 2010-03-30 12:56 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
patch_1 - v1 - adds lock scrollbars concept to ScrollView (21.59 KB, patch) 2010-04-01 13:21 PDT, Antonio Gomes	hyatt: review- tonikitoo: commit-queue-	Details Formatted Diff Diff
patch 1 - v2 - addressed feedback from hyatt on IRC (24.56 KB, patch) 2010-04-04 20:11 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
patch_1 - v2.1 - same as "patch_1 - v2" rebased with ToT (24.80 KB, patch) 2010-04-05 14:15 PDT, Antonio Gomes	tonikitoo: commit-queue-	Details Formatted Diff Diff
(committed r57278) patch_2 - v1 - add stubs for Mac, Gtk, Wx and Win DRTs to implement this method (8.23 KB, patch) 2010-04-05 14:16 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
patch_1 - v2.2 - attempt to build on Mac (same as "patch_1 - v2.1") (25.42 KB, patch) 2010-04-06 08:33 PDT, Antonio Gomes	hyatt: review- tonikitoo: commit-queue-	Details Formatted Diff Diff
patch_1 - v3 - addressed discussion with dhyatt on IRC (25.75 KB, patch) 2010-04-07 08:12 PDT, Antonio Gomes	tonikitoo: commit-queue-	Details Formatted Diff Diff
patch_1 - v3.1 - addressed discussion with dhyatt on IRC + rebased to ToT (25.79 KB, patch) 2010-04-07 08:26 PDT, Antonio Gomes	tonikitoo: commit-queue-	Details Formatted Diff Diff
(committed r57277) patch_1 - v3.2 - addressed discussion with dhyatt on IRC + rebased to ToT + builds in gtk (26.52 KB, patch) 2010-04-07 10:13 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Ismail Donmez

Comment 1 2009-11-18 04:12:57 PST

I can reproduce this with latest qtwebkit 4.6 snapshot.

Antonio Gomes

Comment 2 2010-01-19 11:15:57 PST

raising the importance, since it regresses a previously working API-related code. I can investigate ...

Antonio Gomes

Comment 3 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

Kenneth Rohde Christiansen

Comment 4 2010-03-26 09:41:21 PDT

Please write revision instead. It seems to be r50665

Antonio Gomes

Comment 5 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

Antonio Gomes

Comment 6 2010-03-30 12:56:06 PDT

Created attachment 52071 [details] WIP - it works but is not to be reviewed or landed

Antonio Gomes

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

Antonio Gomes

Comment 8 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?

Eric Seidel (no email)

Comment 9 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

Dave Hyatt

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

Antonio Gomes

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

Antonio Gomes

Comment 12 2010-04-05 14:15:08 PDT

Created attachment 52567 [details] patch_1 - v2.1 - same as "patch_1 - v2" rebased with ToT

Antonio Gomes

Comment 13 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

Antonio Gomes

Comment 14 2010-04-05 14:16:57 PDT

@hyatt, could you help me w/ this two (complementary) patches?

Antonio Gomes

Comment 15 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")

Dave Hyatt

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

Antonio Gomes

Comment 17 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

Antonio Gomes

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

Antonio Gomes

Comment 19 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

WebKit Review Bot

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

WebKit Review Bot

Comment 21 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

Antonio Gomes

Comment 22 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?

WebKit Review Bot

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

Dave Hyatt

Comment 24 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

Antonio Gomes

Comment 25 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!

Antonio Gomes

Comment 26 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!

Simon Hausmann

Comment 27 2010-04-09 00:37:33 PDT

<cherry-pick-for-backport: 57277>

Simon Hausmann

Comment 28 2010-04-09 00:38:02 PDT

<cherry-pick-for-backport: 57278>

Simon Hausmann

Comment 29 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

Note You need to log in before you can comment on or make changes to this bug.