Bug 16520 – [GTK] Acid2 has scrollbar; doesn't quite pass the test

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alp Toker]

Acid2 rendering works but the scrollbar is expected to be hidden since the test sets overflow: hidden -- however it's still visible in the GTK+ port.

A brief look suggested the issue was not in ChromeClientGtk but rather in or around ScrollViewGtk.

[Bert JW Regeer]

16526

is the same thing. Happens on Mac OS  X as well, not limited to just GTK.

[Mark Rowe (bdash)]

Bug 16526 is not related to this at all.

[Marco Barisione]

The problem is visible when you are using real GTK scrollbars but not when you are using ScrollViewScrollbar (such as when the page with overflow:hidden is inside an iframe).

Fixing the overflow:hidden bug is easy, you only have to set the GtkAdjustament so that the GtkScrolledWindow thinks that everything fits in a the page.

Instead it isn't so easy to fix the problem when using overflow:scroll as you cannot easily force the scrolled window to have the scroll bars even if not needed.

I can see two solutions:
1) Emit a signal from the webview to ask for a change in the scroll bar visibility policy. This sucks because you have to add code everytime you use a WebView.
2) Try to access the parent and if it's a scrolled view change the scrollbar policy. This sucks because it's just a workaround.

Suggestions?

[Marco Barisione]

Created attachment 19862 [details]
Possible fix for the problem with overflow:hidden but not with overflow:scroll

[Jan Alonzo]

Created attachment 20715 [details]
acid 2 and regression fix

This patch makes the test pass. Also fixes a regression where new pages maintain the last page's scroll position instead of resetting the position to the top of the page.

[Alp Toker]

Comment on attachment 20715 [details]
acid 2 and regression fix

The frameView->setGtkAdjustments() tells the WebView to use the provided GtkScrolledWindow. Without it, WebKit falls back to its own scrollbars which isn't what we want (but they do work better).

This also causes a hang when resizing http://www.google.com/

Not sure this is the right approach.

[Jan Alonzo]

(In reply to comment #6)
> (From update of attachment 20715 [details] [edit])
> The frameView->setGtkAdjustments() tells the WebView to use the provided
> GtkScrolledWindow. Without it, WebKit falls back to its own scrollbars which
> isn't what we want (but they do work better).

Hi alp. So the gtk port's is different from qt or win in this regard (removing those two lines matches the qt and win's impl for example)? From what I understand that line gets the WebView's adjustment and uses that Adjustment to the new FrameView (hence when you go to a new page the scroll position doesn't move).

Am I missing something? Do you have any idea where I should be looking at?

> 
> This also causes a hang when resizing http://www.google.com/

I doesn't hang here :-\ .

> 
> Not sure this is the right approach.

Thanks anyway. Any pointers will surely help.

[Marco Barisione]

Alp, the problem is that there is not way (or better, no clean way) to fully control the gtk scrollbars from inside the WebView.

Do we really need to use the scroll bars created by the scrolled view?

[Alp Toker]

(In reply to comment #8)
> Alp, the problem is that there is not way (or better, no clean way) to fully
> control the gtk scrollbars from inside the WebView.
> 
> Do we really need to use the scroll bars created by the scrolled view?
> 

It's a design decision to follow GTK+ convention as closely as possible. If the widget was created within a GtkScrolledWindow we need to respect that and can deliver chrome changes by getting at the container widget and conditionally modifying the scrollbar policy in the case that it's a GtkScrolledWindow.

[Marco Barisione]

(In reply to comment #9)
> It's a design decision to follow GTK+ convention as closely as possible. If the
> widget was created within a GtkScrolledWindow we need to respect that and can
> deliver chrome changes by getting at the container widget and conditionally
> modifying the scrollbar policy in the case that it's a GtkScrolledWindow.

Don't you think it's a bit hacky? IMO if we implement it we should change the policy type only if it was set to GTK_POLICY_AUTOMATIC, so we don't override the explicit request to have or not to have the scrollbars.

What should I do in the case of overflow:hidden? Modify the adjustament (it seems to always work well and can be used even if we are not using a scrolled window but it could break something) or use the same trick used for overflow:scroll?

[Holger Freyther]

(In reply to comment #10)

> Don't you think it's a bit hacky?

When I was implementing it I looked at GtkScrolledWindow and http://library.gnome.org/devel/gtk/2.6/GtkWidget.html#gtk-widget-set-scroll-adjustments. So no I don't think it is hacky but the only documented way to implement scrolling. I'm always willing to update my knowledge and change my mind.

> What should I do in the case of overflow:hidden? Modify the adjustament (it
> seems to always work well and can be used even if we are not using a scrolled
> window but it could break something) or use the same trick used for
> overflow:scroll?

WebCore should handle it. In fact the only thing that changed is that the GtkAdjustmets get set at a later point in time (e.g. after a load has started). But even then updateScrollbars is called. So maybe one must force a layout again?

To reiterate. It is about time to use the LayoutTests of WebKit, there are tests for parts of ACID2, e.g. I wonder if I should ask for a reduction of this bug or if we already have one in the LayoutTests. If we have a reduction and the result is different with the FrameView change and without it I can look at it.

The two patches so far don't look quite right. The ScrollBar code is 'shared' with the windows port and it is unlikely we need that change but windows doesn't (only if Safari is broken as well). Not setting the GtkAdjustments is not the right thing to do for the already existing comments.

[Dirk Schulze]

This seems to be fixed now. Don't know what fixed it but I'm closing the bug.