Bug 135506 – Scrolling with spacebar on a page with fixed header breaks reading flow

• Component: Layout and Rendering
• Status: RESOLVED FIXED

This text is taken largely from a reddit comment (originating here: http://www.reddit.com/r/mac/comments/2c7qwu/which_browser_do_you_use/cjctwum).

---- 

When using spacebar to scroll a website that has a fixed header, Firefox scrolls the "right" amount, while safari scrolls the "wrong" amount. 

Take for example this webpage which uses a fixed header: http://nautil.us/issue/15/turbulence/nasa-is-going-to-dip-this-cup-into-the-suns-corona. In firefox, scroll down so that the text "and understanding the solar" is at the foot of the page, as in this image: https://www.dropbox.com/s/hpwbhnnd2r3e8tz/firefoxstart.png. Now hit space. Firefox scrolls just enough so that the same text is then visible (mostly) at the top of the page, allowing you to continue reading from where you were previously (shown in this image:(https://www.dropbox.com/s/lrze7l1ifx8iqsx/firefxend.png).

Now for safari (and the same is true of Chrome, iirc). Do the same setup and repeat the steps. Here is the start: https://www.dropbox.com/s/w8tsminoefnn98p/safaristart.png). And here we are after hitting space: https://www.dropbox.com/s/h8zssngfke6xnio/safariend.png. Unfortunately, now we've missed some text and have to manually adjust the page to get back to where we were: https://www.dropbox.com/s/cwpbgxi5auaq2y3/safarimissed.png. This, at least in my experience, is really frustrating and breaks the reading experience.

Now, I'm not saying that this is Safari's fault. I'm just saying it's important to me, and Firefox works as I would like it to.

[Benjamin Poulain]

Created attachment 236059 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 236059 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:3199
> +    for (const auto slot : *positionedObjects) {

slot -> positionedObject

> Source/WebCore/page/FrameView.cpp:3218
> +        if (fixedRectInView.y() == unobscuredContentRect.y())
> +            topObscuredArea = std::max(topObscuredArea, fixedRectInView.height());
> +        else if (fixedRectInView.maxY() == unobscuredContentRect.maxY())
> +            bottomObscuredArea = std::max(bottomObscuredArea, fixedRectInView.height());
> +    }
> +    return std::max<float>(0, step - topObscuredArea - bottomObscuredArea);

I think the behavior when the fixed object covers most of the view is undesirable. I would limit this to fixed objects which are obviously bars (maybe < 20% of view height).

Also, there are sites with fully transparent position:fixed that cover much of the page, and this would make scrolling really funky on such pages.

With Benjamin's patch, the following webpage does not scroll (at least visually) using spacebar: https://medium.com/matter/why-are-dope-addicted-disgraced-doctors-running-our-drug-trials-aff6d20843bf

[Benjamin Poulain]

(In reply to comment #3)
> With Benjamin's patch, the following webpage does not scroll (at least visually) using spacebar: https://medium.com/matter/why-are-dope-addicted-disgraced-doctors-running-our-drug-trials-aff6d20843bf

In that page, there is a canvas element with fixed positioning taking 100% of the page. It weird how fragile Firefox's heuristic is, it is very easy to break.

The new version should fix this case as the scrolling is a minimum of ScrollBar::minFractionToStepWhenPaging().

[Benjamin Poulain]

Created attachment 236357 [details]
Patch

[Benjamin Poulain]

Quick update addressing the comments.

I'll have to update the tests to account for the minimum vertical scrolling. Some cleanup also needed.

(In reply to comment #4)
> (In reply to comment #3)
> > With Benjamin's patch, the following webpage does not scroll (at least visually) using spacebar: https://medium.com/matter/why-are-dope-addicted-disgraced-doctors-running-our-drug-trials-aff6d20843bf
> 
> In that page, there is a canvas element with fixed positioning taking 100% of the page. It weird how fragile Firefox's heuristic is, it is very easy to break.
> 
> The new version should fix this case as the scrolling is a minimum of ScrollBar::minFractionToStepWhenPaging().

Cool. I'll give it a whirl.

BTW, I recall from my tinkering with compiled languages that when recompiling to include small changes, the compiler didn't necessarily have to recompile to whole tree. Is this possible with WebKit? I'm using ./Tools/Scripts/build-webkit to, well, build WebKit, but it takes around an hour to complete.

Ta.

[Build Bot]

Comment on attachment 236357 [details]
Patch

Attachment 236357 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4912411433762816

New failing tests:
scrollbars/scrolling-by-page-accounting-top-fixed-elements-on-keyboard-spacebar.html
scrollbars/scrolling-backward-by-page-accounting-bottom-fixed-elements-on-keyboard-spacebar.html

[Build Bot]

Created attachment 236361 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

The new patch fixes the aforementioned problems.

[Benjamin Poulain]

Created attachment 236702 [details]
Patch

[Build Bot]

Comment on attachment 236702 [details]
Patch

Attachment 236702 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5974821993185280

New failing tests:
fast/events/scrollbar-double-click.html

[Build Bot]

Created attachment 236705 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 236702 [details]
Patch

Attachment 236702 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5178976364396544

New failing tests:
fast/events/scrollbar-double-click.html

[Build Bot]

Created attachment 236708 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 236702 [details]
Patch

Attachment 236702 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4896004054712320

New failing tests:
fast/events/scrollbar-double-click.html

[Build Bot]

Created attachment 236726 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Simon Fraser (smfr)]

Comment on attachment 236702 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:3233
> +        if (positionedObject->style().position() != FixedPosition)
> +            continue;
> +
> +        FloatQuad contentQuad = positionedObject->absoluteContentQuad();
> +        if (!contentQuad.isRectilinear())
> +            continue;

I still think this heuristic needs to be smarter. It should ignore visibility:hidden and opacity:0 fixed elements, for one thing. I think you should also take into account fixed elements that have negative top, but whose maxY is inside the viewport (and similarly for the bottom); many pages do this to have bars that move in and out.

[Benjamin Poulain]

Comment on attachment 236702 [details]
Patch

Clearing the review flag. I'll update when I get some time.

[Benjamin Poulain]

Created attachment 237015 [details]
Patch

[Benjamin Poulain]

Created attachment 237017 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 237017 [details]
Patch

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

> Source/WebCore/platform/Scrollbar.h:102
> +    static int pageStep(int viewWidthOrHeight, int contentWidthOrHeight) { return std::max(std::max<int>(lroundf(viewWidthOrHeight * Scrollbar::minFractionToStepWhenPaging()), lroundf(contentWidthOrHeight - Scrollbar::maxOverlapBetweenPages())), 1); }

You added contentWidthOrHeight but at all the call sites you're passing clientWidth/height, the same as the first param?

[Benjamin Poulain]

(In reply to comment #22)
> (From update of attachment 237017 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237017&action=review
> 
> > Source/WebCore/platform/Scrollbar.h:102
> > +    static int pageStep(int viewWidthOrHeight, int contentWidthOrHeight) { return std::max(std::max<int>(lroundf(viewWidthOrHeight * Scrollbar::minFractionToStepWhenPaging()), lroundf(contentWidthOrHeight - Scrollbar::maxOverlapBetweenPages())), 1); }
> 
> You added contentWidthOrHeight but at all the call sites you're passing clientWidth/height, the same as the first param?

All except FrameView. The others call sites do not "restrict" the content area.

[Simon Fraser (smfr)]

Comment on attachment 237017 [details]
Patch

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

r- for the Scrollbar::pageStep confusion.

> Source/WebCore/page/FrameView.cpp:3239
> +        if (style.position() != FixedPosition || style.visibility() == HIDDEN || style.opacity() <= 0)

opacity should never be < 0.

> Source/WebCore/page/FrameView.cpp:3250
> +        if (fixedRectInView.width() < unobscuredContentRect.width())
> +            continue;

What about top bars that have a bit of space on each side?

> Source/WebCore/page/FrameView.cpp:3252
> +        if (fixedRectInView.y() == unobscuredContentRect.y())

Some pages will have fixed elements that are partially off the top, so I think it would be better to test whether unobscuredContentRect.y() falls vertically inside the fixed element rect.

> Source/WebCore/page/FrameView.cpp:3254
> +        else if (fixedRectInView.maxY() == unobscuredContentRect.maxY())

Ditto.

> Source/WebCore/platform/ScrollAnimator.cpp:118
> -                deltaY = Scrollbar::pageStepDelta(m_scrollableArea->visibleHeight());
> +                int visibleHeight = m_scrollableArea->visibleHeight();
> +                deltaY = Scrollbar::pageStepDelta(visibleHeight);

This change doesn't seem necessary.

> Source/WebCore/platform/ScrollAnimator.cpp:129
> -                deltaX = Scrollbar::pageStepDelta(m_scrollableArea->visibleWidth());
> +                int visibleWidth = m_scrollableArea->visibleWidth();
> +                deltaX = Scrollbar::pageStepDelta(visibleWidth);

Ditto.

> Source/WebCore/platform/ScrollView.cpp:701
> +        int pageStep = Scrollbar::pageStep(clientHeight, clientHeight);

It's really confusing to pass clientHeight again as the 2nd param. I don't know what this is trying to do.

[Benjamin Poulain]

Created attachment 237253 [details]
Patch

[Benjamin Poulain]

Comment on attachment 237253 [details]
Patch

Clearing flags on attachment: 237253

Committed r173074: <http://trac.webkit.org/changeset/173074>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.