Bug 25646 - [GTK] onscroll event is never sent
Summary: [GTK] onscroll event is never sent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 25889
  Show dependency treegraph
 
Reported: 2009-05-08 09:47 PDT by Holger Freyther
Modified: 2009-05-26 22:27 PDT (History)
2 users (show)

See Also:


Attachments
Make scrolling go through Scrollbar to get the ScrollbarClient invoked (16.47 KB, patch)
2009-05-08 09:48 PDT, Holger Freyther
no flags Details | Formatted Diff | Diff
Completely kill the "adjustment" (18.37 KB, patch)
2009-05-10 22:46 PDT, Holger Freyther
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Holger Freyther 2009-05-08 09:47:54 PDT
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...
Comment 1 Holger Freyther 2009-05-08 09:48:58 PDT
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.
Comment 2 Gustavo Noronha (kov) 2009-05-09 10:46:35 PDT
>         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 =(.
Comment 3 Xan Lopez 2009-05-10 10:59:22 PDT
(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!
Comment 4 Holger Freyther 2009-05-10 22:46:12 PDT
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.
Comment 5 Dave Hyatt 2009-05-24 23:28:48 PDT
Comment on attachment 30172 [details]
Completely kill the "adjustment"

r=me
Comment 6 Holger Freyther 2009-05-26 22:27:18 PDT
Landed in r44177.