CLOSED FIXED 29431
[Qt] REGRESSION:(r50665) QWebFrame::setScrollBarPolicy(Qt::Vertical,Qt::ScrollBarAlwaysOff) has no effect.
https://bugs.webkit.org/show_bug.cgi?id=29431
Summary [Qt] REGRESSION:(r50665) QWebFrame::setScrollBarPolicy(Qt::Vertical,Qt::Scrol...
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
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-
patch 1 - v2 - addressed feedback from hyatt on IRC (24.56 KB, patch)
2010-04-04 20:11 PDT, Antonio Gomes
no flags
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-
(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
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-
patch_1 - v3 - addressed discussion with dhyatt on IRC (25.75 KB, patch)
2010-04-07 08:12 PDT, Antonio Gomes
tonikitoo: commit-queue-
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-
(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
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
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
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.