Bug 25646 – [GTK] onscroll event is never sent

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Holger Freyther]

The current main frame/frame view of WebKitWebView is currently never sending a onscroll event. This is due the ScrollView::valueChanged code being mostly dead code...

[Holger Freyther]

Created attachment 30133 [details]
Make scrolling go through Scrollbar to get the ScrollbarClient invoked

proposed approach for the problem, only halfway through though. Please comment anyway.

[Gustavo Noronha (kov)]

>         Using the wrapped GtkAdjustment approach we might be able to just have
>         special case for the ScrollView::createScrollbar to create these
>         scrollbars...
[...]
> PassRefPtr<ScrollbarGtk> ScrollbarGtk::createScrollbar(ScrollbarClient* client, ScrollbarOrientation orientation, GtkAdjustment* adj)
> {
>     return adoptRef(new ScrollbarGtk(client, orientation, adj));
> }

And using this we can ditch the createNativeScrollbar, you mean?

> -        /*
> -         * disable the scrollbars (if we have any) as the GtkAdjustment over
> -         */

I like the patch in general, it's a good cleanup, but this comment has really blown off my mind while I was reading through this code last week, and I believe you could perhaps make it clearer at this point why we need to disable the scrollbars here:

> +    } else {
> +        m_horizontalAdjustment = ScrollbarGtk::createScrollbar(this, HorizontalScrollbar, hadj);
> +        m_verticalAdjustment = ScrollbarGtk::createScrollbar(this, VerticalScrollbar, vadj);
>          setHasVerticalScrollbar(false);
>          setHasHorizontalScrollbar(false);

[...]

> +        m_horizontalAdjustment->setSteps(visibleWidth() / 10.0, visibleWidth() * 0.9, 1);
> +        m_horizontalAdjustment->setProportion(visibleWidth(), contentsWidth());
> +        m_horizontalAdjustment->setCurrentPos(scroll.width());

Xan has done some work recently on the scrolling code to use the WebCore constants instead of magic numbers, which I think we should use here as well; See r43390.

> +        m_verticalAdjustment->setSteps(visibleHeight() / 10.0, visibleHeight() * 0.9, 1);
> +        m_verticalAdjustment->setProportion(visibleHeight(), contentsHeight());
> +        m_verticalAdjustment->setCurrentPos(scroll.height());

Same here.

> +// Wrap a GtkAdjustment into a WebCore::Scrollbar to properly use the ScrollbarClient
> +// for things like valueChanged.
> +ScrollbarGtk::ScrollbarGtk(ScrollbarClient* client, ScrollbarOrientation orientation, GtkAdjustment* adjustment)
> +    : Scrollbar(client, orientation, RegularScrollbar)
> +    , m_adjustment(adjustment)
> +{
> +    // reset the adjustment to a known state
> +    m_adjustment->lower = 0;
> +    if (m_adjustment->value != 0) {
> +        m_adjustment->value = 0;
> +        gtk_adjustment_value_changed(m_adjustment);
> +    }

Is this wanted? I'm thinking of a javascript call that forces a scrollbar to appear, by using the overflow CSS property, for instance. Will it make the scrolled content go to the top?

> +    // We have nothing to show as we are solely operating on the GtkAdjustment
> +    resize(0, 0);

I don't get it =(.

[Xan Lopez]

(In reply to comment #2)
> And using this we can ditch the createNativeScrollbar, you mean?
> 
> > -        /*
> > -         * disable the scrollbars (if we have any) as the GtkAdjustment over
> > -         */
> 
> I like the patch in general, it's a good cleanup, but this comment has really
> blown off my mind while I was reading through this code last week, and I
> believe you could perhaps make it clearer at this point why we need to disable
> the scrollbars here:

That's because when you set the adjustments you are telling the View that you'll handle the scrolling yourself (for example, through a GtkScrolledWindow), so you want to disable WebKit's scrollbars. I'm not sure why Holger is removing the comment though...


> > +        m_horizontalAdjustment->setSteps(visibleWidth() / 10.0, visibleWidth() * 0.9, 1);
> > +        m_horizontalAdjustment->setProportion(visibleWidth(), contentsWidth());
> > +        m_horizontalAdjustment->setCurrentPos(scroll.width());
> 
> Xan has done some work recently on the scrolling code to use the WebCore
> constants instead of magic numbers, which I think we should use here as well;
> See r43390.

/me nods (although the WebCore constants are more 'magic numbers' than what we were using :D)

I'll try to look at this tomorrow!

[Holger Freyther]

Created attachment 30172 [details]
Completely kill the "adjustment"

The only special case we have in GTK+ after this patch is that we have a custom ::createScrollbar implementation that will create a ScrollbarGtk to operate on the external GtkAdjustment. This way we will go through the normal Scrollbar paths and fix the sendScrollEvent bug.

[Dave Hyatt]

Comment on attachment 30172 [details]
Completely kill the "adjustment"

r=me

[Holger Freyther]

Landed in r44177.