gtkscrolledwindow.c (from upstream GTK) converts the scroll units received from GDK_SCROLL_SMOOTH to pixels as follows: > scroll_unit = pow (page_size, 2.0 / 3.0); In our current scrolling implementation, we simply multiply GDK_SCROLL_SMOOTH values by 3 and interpret them as line deltas. This is inconsistent with other GTK apps- we should consider converting to pixels using GTK's formula for delta pages.

This patch modifies delta page scrolling to allow platform page scroll size overrides via look and feel, allowing us to act like other GTK apps. Karl, is this worth pursuing? It replaces the ambiguous 3x event -> line multiplier code.

Comment on attachment 8623327 [details] [diff] [review] WIP scroll units on gtk Review of attachment 8623327 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gtk/nsLookAndFeel.cpp @@ +1264,5 @@ > +nsSize > +nsLookAndFeel::GetPageScrollAmountImpl(const nsSize& aPageSize) > +{ > + // See get_scroll_unit in gtkscrollablewindow.c. > + return nsSize(pow(aPageSize.width, 2.0 / 3.0), You should probably convert these values to CSS pixels first, otherwise your results will be completely off. nsSize values are in app units, there are 60 app units in a CSS pixel.

(In reply to Markus Stange [:mstange] from comment #2) > Comment on attachment 8623327 [details] [diff] [review] > WIP scroll units on gtk > > Review of attachment 8623327 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/gtk/nsLookAndFeel.cpp > @@ +1264,5 @@ > > +nsSize > > +nsLookAndFeel::GetPageScrollAmountImpl(const nsSize& aPageSize) > > +{ > > + // See get_scroll_unit in gtkscrollablewindow.c. > > + return nsSize(pow(aPageSize.width, 2.0 / 3.0), > > You should probably convert these values to CSS pixels first, otherwise your > results will be completely off. nsSize values are in app units, there are 60 > app units in a CSS pixel. You're right, the types are wrong (should be nsIntSize) but the values are correct here. I'm thinking that it might be a good idea to represent the wheel events as line deltas, and change the LookAndFeel method to override the line height based on the page height on GTK. That way, we can avoid implementing delta page accumulation for legacy wheel events. The wheel events are much closer to lines than pages anyway.

(In reply to Andrew Comminos [:acomminos] from comment #3) > (In reply to Markus Stange [:mstange] from comment #2) > > Comment on attachment 8623327 [details] [diff] [review] > > WIP scroll units on gtk > > > > Review of attachment 8623327 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: widget/gtk/nsLookAndFeel.cpp > > @@ +1264,5 @@ > > > +nsSize > > > +nsLookAndFeel::GetPageScrollAmountImpl(const nsSize& aPageSize) > > > +{ > > > + // See get_scroll_unit in gtkscrollablewindow.c. > > > + return nsSize(pow(aPageSize.width, 2.0 / 3.0), > > > > You should probably convert these values to CSS pixels first, otherwise your > > results will be completely off. nsSize values are in app units, there are 60 > > app units in a CSS pixel. > > You're right, the types are wrong (should be nsIntSize) but the values are > correct here. Are you sure about that? I expect they'll be off by a factor of ~4: (60*x)^(2/3)/60 == 60^(2/3)/60 * x^(2/3) == x^(2/3) / 60^(1/3) == x^(2/3) / 3.915 > I'm thinking that it might be a good idea to represent the wheel events as > line deltas, and change the LookAndFeel method to override the line height > based on the page height on GTK. That way, we can avoid implementing delta > page accumulation for legacy wheel events. Interesting idea. Definitely worth a try, I think.

Oh, I see it now. You were creating an nsSize around pixel values and pulling them out afterwards. Ok.

Here's a cleaner patch that uses line scrolling deltas, converted into pixels in EventStateManager using the system look and feel.

Use nsGfxScrollFrame for viewport size when available.

Comment on attachment 8623847 [details] [diff] [review] Implement page size based scroll tick support for GTK. Review of attachment 8623847 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.

Comment on attachment 8623847 [details] [diff] [review] Implement page size based scroll tick support for GTK. Looks good to me thanks. >+ aTickSize.width = nsPresContext::CSSPixelsToAppUnits( >+ (float)pow(pageSizeInCSSPixels.width, 2.0 / 3.0)); powf(pageSizeInCSSPixels.width, 2.0f / 3.0f)) would make unnecessary the cast to resolve the overload.

Here's an updated patch using powf instead. Thanks!

Comment on attachment 8624307 [details] [diff] [review] Implement page size based scroll tick support for GTK. First of all, cannot always use DOM_DELTA_PAGE? Then, delta value should be able to be computed in widget at dispatching wheel events. But it may cause some websites which handle wheel event by themselves. If you believe we shouldn't use DOM_DELTA_PAGE, I'll support you for compatibility with other platforms. However, in strictly speaking, we should use DOM_DELTA_PAGE in this case. Additionally, it might not be simple. In widget, you set non-line amounts into deltaX/deltaY. However, you don't correct it in EventStateManager::PreHandleEvent. So, web contents will receive wrong scroll amount. You need to modify the deltaX and deltaY in it with the nearest scrollable frame from the event target. However, default action will scroll different scrollable frame for mouse wheel transaction. Therefore, you need to store the original delta values in EventStateManager or anywhere... >+ // Attempt to use the line size of the system look and feel. >+ if (aEvent->deltaMode == nsIDOMWheelEvent::DOM_DELTA_LINE) { >+ nsSize tickSize; >+ nsSize pageSize = aScrollableFrame ? >+ aScrollableFrame->GetScrollPortRect().Size() : >+ aPresContext->GetVisibleArea().Size(); >+ if (NS_SUCCEEDED(LookAndFeel::GetScrollTickSize(pageSize, tickSize))) { >+ return tickSize; >+ } I don't like here because this works with only GTK3. However, I don't like to use #ifdef nor prefs. Could you define a static variable here like sGetScrollTickSizeImplemented or something? Once the result is NS_ERROR_NOT_IMPLEMENTED, this block should be skipped. And also I don't like you send nsSize. Please compute CSS pixels and use nsIntSize. app units shouldn't be treated in widget. >+// static >+nsresult >+LookAndFeel::GetScrollTickSize(const nsSize& aPageSize, nsSize& aTickSize) >+{ >+ return nsLookAndFeel::GetInstance()->GetScrollTickSizeImpl(aPageSize, aTickSize); >+} I think that this should be renamed better. This is not clear that this is used for WidgetWheelEvent nor this computes the scroll pixels with pseudo line height which is computed from page size. So, keeping using DOM_DELTA_LINE makes EventStateManager more complicated. So, I strongly recommend that you should use DOM_DELTA_PAGE if we can ignore the websites which *forget* to handle DOM_DELTA_PAGE.

Thanks for the feedback. What are your thoughts on defining a 'custom' unit defined by the system look and feel sent in a WidgetWheelEvent that gets translated into a DOM_DELTA_PIXEL event in EventStateManager::PreHandleEvent (based on the frame's viewport size, of course)? That way, we would only have to query the system look and feel when we got that custom type of event, and it'll be accumulated into line and page deltas much like any other pixel scrolling event. The DOM shouldn't see anything different.

Sorry for the delay due to missed to catch the ni request. (In reply to Andrew Comminos [:acomminos] from comment #12) > Thanks for the feedback. What are your thoughts on defining a 'custom' unit > defined by the system look and feel sent in a WidgetWheelEvent that gets > translated into a DOM_DELTA_PIXEL event in EventStateManager::PreHandleEvent > (based on the frame's viewport size, of course)? That way, we would only > have to query the system look and feel when we got that custom type of > event, and it'll be accumulated into line and page deltas much like any > other pixel scrolling event. The DOM shouldn't see anything different. I don't think we need a new unit. Please use DOM_DELTA_PAGE at least internally. If some webpages will be broken by the change, EventStateManager::PreHandleEvent can convert it to DOM_DELTA_PIXEL. But I guess that DOM_DELTA_PAGE is the best unit for webpages too since the pixels computed from viewport is too large for scrollable elements such as <textarea>, <iframe> and etc. I'd like to use DOM_DELTA_PAGE for now, and we can watch bug reports. Smaug, any ideas? See comment 0 and comment 11. I think that according to the GTK's native behavior, we should set DOM_DELTA_PAGE to WidgetWheelEvent in the widget for GTK. But it has some risk.

Oops, see the previous comment :-(

The 2/3 power heuristics typically result in scroll units of around 1/10th of a page. It approaches 1/2 a page at 10 pixels.