[webkit-reviews] review denied: [Bug 135506] Scrolling with spacebar on a page with fixed header breaks reading flow : [Attachment 237017] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 135506: Scrolling with spacebar on a page with fixed header breaks reading
flow
https://bugs.webkit.org/show_bug.cgi?id=135506

Attachment 237017: Patch
https://bugs.webkit.org/attachment.cgi?id=237017&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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.