Bug 34371 - REGRESSION (r53718): When scrolling a tall window by page, the overlap between pages is too big
: REGRESSION (r53718): When scrolling a tall window by page, the overlap betwee...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
: InRadar, Regression
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-30 00:27 PST by mitz@webkit.org
Modified: 2012-11-30 12:31 PST (History)
9 users (show)

See Also:


Attachments
Cap the overlap amount at 40 pixels (9.22 KB, patch)
2010-01-30 00:30 PST, mitz@webkit.org
no flags Details | Formatted Diff | Diff
Allow ScrollbarTheme to cap the overlap between pages, and set a cap of in ScrollbarThemeMac (17.70 KB, patch)
2010-01-30 01:31 PST, mitz@webkit.org
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz@webkit.org 2010-01-30 00:27:41 PST
<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.
Comment 1 Peter Kasting 2010-01-30 00:30:39 PST
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".
Comment 2 mitz@webkit.org 2010-01-30 00:30:53 PST
Created attachment 47758 [details]
Cap the overlap amount at 40 pixels
Comment 3 Peter Kasting 2010-01-30 00:32:12 PST
(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).")
Comment 4 mitz@webkit.org 2010-01-30 00:34:42 PST
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.
Comment 5 Peter Kasting 2010-01-30 00:37:26 PST
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.
Comment 6 Eric Seidel 2010-01-30 00:39:15 PST
Attachment 47758 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/220331
Comment 7 Peter Kasting 2010-01-30 00:40:11 PST
(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.)
Comment 8 mitz@webkit.org 2010-01-30 01:31:07 PST
Created attachment 47759 [details]
Allow ScrollbarTheme to cap the overlap between pages, and set a cap of in ScrollbarThemeMac
Comment 9 Peter Kasting 2010-01-30 01:34:15 PST
I am happy with this patch in principle.
Comment 10 WebKit Review Bot 2010-01-30 02:00:21 PST
Attachment 47759 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/221255
Comment 11 Eric Seidel 2010-02-01 15:57:12 PST
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
Comment 12 mitz@webkit.org 2010-02-03 22:05:05 PST
<rdar://problem/7611154>
Comment 13 Simon Fraser (smfr) 2010-02-04 08:12:07 PST
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.
Comment 14 John Sullivan 2010-02-04 08:53:28 PST
Maybe "overlap" for "amountToKeep"?
Comment 15 mitz@webkit.org 2010-02-04 09:37:02 PST
<http://trac.webkit.org/projects/webkit/changeset/54345>.
Comment 16 Ryosuke Niwa 2012-03-12 13:42:23 PDT
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
Comment 17 Ryosuke Niwa 2012-03-12 13:48:21 PDT
Rebaselined in http://trac.webkit.org/changeset/110472. Please let me know if you think this rebaseline is incorrect.
Comment 18 mitz@webkit.org 2012-03-12 14:13:23 PDT
This is a side effect of running the tests with ScrollbarThemeMock.
Comment 19 mitz@webkit.org 2012-03-12 14:14:21 PDT
We’ve lost test coverage for all the code in ScrollbarThemeMac :(
Comment 20 Graeme Hewson 2012-11-30 12:24:13 PST
Could this be user-configurable, a user asks. See https://forum.kde.org/viewtopic.php?f=15&t=108930&sid=c585b1db933e0c0be00ccd27a67e3638
Comment 21 Peter Kasting 2012-11-30 12:31:34 PST
(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.