Bug 34371 – REGRESSION (r53718): When scrolling a tall window by page, the overlap between pages is too big

• Component: New Bugs
• Status: RESOLVED FIXED

<http://trac.webkit.org/changeset/53718> changed scroll by page behavior to make the overlap between pages a fraction of the visible height (width when scrolling horizontally). This was motivated by the fixed-size overlap being too big for small areas. However, it resulted in the overlap being too big for big areas. For example, for a visible height of around 1000 pixels, the overlap is 125 pixels, compared to 40 pixels before the change. In Firefox on Mac OS X, the overlap for a similar visible height is only 32 pixels.

[Peter Kasting]

No, the original motivation was the overlap being too small for big areas, making page scrolls hinder readability.  The change was made to match IE which behaves much more nicely than Fx: having a 125 px overlap at 1000 px is exactly what we wanted.

Fixing the excessive overlap in tiny areas was an added bonus, but not the main goal.

FWIW, hyatt's comment in IRC was "oh, my original 40 px change was an attempt to match IE, so if they do 1/8th page, let's do that".

Created attachment 47758 [details]
Cap the overlap amount at 40 pixels

[Peter Kasting]

(From the original bug:

"This means that when we scroll by "one page", we overlap by 40 px.  This is an
extremely small amount -- for any nontrivial window height, barely enough to be
noticeable.  ...

By contrast, IE overlaps by 1/8th of the page height.  This strikes me as much
better for readability (and indeed, when reading on my full-height window, it's
much easier to read without losing my place).")

I am not aware of any application on Mac OS X where the overlap is anywhere near 125 pixels for a window size of 1000 pixels. This makes Safari look broken on Mac OS X.

[Peter Kasting]

Well we certainly don't want to cap at 40 px, that was exactly the problem with the old behavior.  And Chromium on OS X (and all other OSes, of course) wants to keep the current behavior.

This is why I asked about platform-specific behavior and objections on the old bug.

Perhaps we can put in a Safari/Mac-specific override.

[Eric Seidel (no email)]

Attachment 47758 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/220331

[Peter Kasting]

(Though I wonder if everyone would agree that it "looks broken".  It doesn't
look broken to me on my MacBook, or on my 1920-pixel-high fullscreen-browsing
Windows desktop either.)

Created attachment 47759 [details]
Allow ScrollbarTheme to cap the overlap between pages, and set a cap of in ScrollbarThemeMac

[Peter Kasting]

I am happy with this patch in principle.

[WebKit Review Bot]

Attachment 47759 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/221255

[Eric Seidel (no email)]

This will break Gtk:
../../WebCore/platform/gtk/WheelEventGtk.cpp: In constructor ‘WebCore::PlatformWheelEvent::PlatformWheelEvent(GdkEventScroll*)’:
../../WebCore/platform/gtk/WheelEventGtk.cpp:72: error: ‘cScrollbarPixelsPerLineStep’ was not declared in this scope

<rdar://problem/7611154>

[Simon Fraser (smfr)]

Comment on attachment 47759 [details]
Allow ScrollbarTheme to cap the overlap between pages, and set a cap of in ScrollbarThemeMac

> Index: WebCore/platform/Scrollbar.h
> ===================================================================

> +    static int pixelsPerLineStep() { return 40; }
> +    static float minFractionToStepWhenPaging() { return 0.875f; }
> +    static int maxAmountToKeepWhenPaging();

I think I'd like to see "pixels" in the name of this method.I'm not super-keen on "amount to keep" either, but can't think of a better term.

[John Sullivan]

Maybe "overlap" for "amountToKeep"?

<http://trac.webkit.org/projects/webkit/changeset/54345>.

[Ryosuke Niwa]

Comment on attachment 47759 [details]
Allow ScrollbarTheme to cap the overlap between pages, and set a cap of in ScrollbarThemeMac

View in context: https://bugs.webkit.org/attachment.cgi?id=47759&action=review

> LayoutTests/platform/mac/fast/events/scrollbar-double-click-expected.txt:1
> +Scroll offset is 720

It seems like scroll offset is always 700 now:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r110449%20(37984)/fast/events/scrollbar-double-click-pretty-diff.html
http://build.webkit.org/results/Lion%20Intel%20Release%20(Tests)/r110450%20(6234)/fast/events/scrollbar-double-click-pretty-diff.html

[Ryosuke Niwa]

Rebaselined in http://trac.webkit.org/changeset/110472. Please let me know if you think this rebaseline is incorrect.

This is a side effect of running the tests with ScrollbarThemeMock.

We’ve lost test coverage for all the code in ScrollbarThemeMac :(

[Graeme Hewson]

Could this be user-configurable, a user asks. See https://forum.kde.org/viewtopic.php?f=15&t=108930&sid=c585b1db933e0c0be00ccd27a67e3638

[Peter Kasting]

(In reply to comment #20)
> Could this be user-configurable, a user asks. See https://forum.kde.org/viewtopic.php?f=15&t=108930&sid=c585b1db933e0c0be00ccd27a67e3638

That'd be up to the individual port in question.  The patch that was landed here allows ports to control the overlap amount, so it's up to those ports to decide what they want to do.