Bug 69220 – Fix FrameView::scrollElementToRect to take already scrolled amount into consideration

• Component: DOM
• Status: RESOLVED FIXED

[Sadrul Habib Chowdhury]

Fix FrameView::scrollElementToRect to take already scrolled amount into consideration.

[Sadrul Habib Chowdhury]

Created attachment 109415 [details]
patch

I updated the existing layout test to scroll first, so that this fix is covered by the test.

[Alexey Proskuryakov]

What actual bug does this fix?

From the function name, it sounds like it should not be relative to current scroll amount, so this patch seems wrong.

[Sadrul Habib Chowdhury]

(In reply to comment #2)
> What actual bug does this fix?
> 
> From the function name, it sounds like it should not be relative to current scroll amount, so this patch seems wrong.

For context, please see https://bugs.webkit.org/show_bug.cgi?id=68198

The function is expected to move the specified element into the specified rectangular region (where the specified rectangular region represents the visible region in the client), and scroll the frame if needed. The function computes the amount needed to scroll from the top of the page, but does not take into consideration if the page is already scrolled some amount. So it ends up overscrolling when called in an already scrolled page. This patch fixes that.

[Sadrul Habib Chowdhury]

(In reply to comment #2)
> What actual bug does this fix?
> 
> From the function name, it sounds like it should not be relative to current scroll amount, so this patch seems wrong.

The patch makes sure it is not relative to the current scroll amount (please see the updated layout test).

[Fady Samuel]

Comment on attachment 109415 [details]
patch

The really should be using FrameView::scrollTo, rather than scrollBy so that it's no longer relative.

[Sadrul Habib Chowdhury]

(In reply to comment #5)
> (From update of attachment 109415 [details])
> The really should be using FrameView::scrollTo, rather than scrollBy so that it's no longer relative.

I considered switching from scrollBy to setScrollPosition (ScrollView::scrollTo has a NOTE recommending not to be used), but I am not entirely sure they have the exact same behaviour, so I decided to keep the same function, and fix the position instead.

[Alexey Proskuryakov]

> The patch makes sure it is not relative to the current scroll amount (please see the updated layout test).

OK. Bug title could be improved - it says "take already scrolled amount into consideration", while you are making it so that the function actually ignores.

I agree with Fady that it's the use of scrollBy that seems wrong here.

> For context, please see https://bugs.webkit.org/show_bug.cgi?id=68198

There is no rationale in that bug. It's about moving some Chromium code into WebCore, but I don't see an explanation of why this code is needed in the first place. It's a pity that only Chromium developers looked at that bug.

A function that has the goal to make something visible should have "reveal" in its name to match the rest of WebCore. As opposed to scrolling to a point, a "reveal" function is allowed to make some platform specific decisions as to which side of the visible area the element will appear at, and how far from the side.

[Varun Jain]

(In reply to comment #7)
> > The patch makes sure it is not relative to the current scroll amount (please see the updated layout test).
> 
> OK. Bug title could be improved - it says "take already scrolled amount into consideration", while you are making it so that the function actually ignores.
> 
> I agree with Fady that it's the use of scrollBy that seems wrong here.
> 
> > For context, please see https://bugs.webkit.org/show_bug.cgi?id=68198
> 
> There is no rationale in that bug. It's about moving some Chromium code into WebCore, but I don't see an explanation of why this code is needed in the first place. It's a pity that only Chromium developers looked at that bug.
> 
> A function that has the goal to make something visible should have "reveal" in its name to match the rest of WebCore. As opposed to scrolling to a point, a "reveal" function is allowed to make some platform specific decisions as to which side of the visible area the element will appear at, and how far from the side.

Hi Alexey,

The original CL (not the refactoring is this: https://bugs.webkit.org/show_bug.cgi?id=68192

The rational was to provide an API for the embedder to scroll an element on the page to a non-hidden area of the screen (note that this is not achievable by Element::scrollIntoViewIfNeeded since the view area may be partially obscured. In the original CL, I was instructed by the reviewer to move the scrolling code into webcore. There are valid reasons for this: 1. the WebKit API is not the place to add logic and should only act as an entry point, 2. Having it in WebCore makes it testable using window.internals. I had a discussion with the reviewer and we found that FrameView would be the best place to put this.

[Simon Fraser (smfr)]

Comment on attachment 109415 [details]
patch

I agree that scrollTo would be more useful here. Why was scrollElementToRect() added? It's already been found to be wrong in 2 different ways.

[Varun Jain]

(In reply to comment #9)
> (From update of attachment 109415 [details])
> I agree that scrollTo would be more useful here. Why was scrollElementToRect() added? It's already been found to be wrong in 2 different ways.

Hi Simon, please read my comment above for why scrollElementToRect was added. In addition to the reasons mentioned above, since FrameView already contains things like scrollToAnchor, scrollToFragment, it seemed like the best place to put scrollElementToRect

[Sadrul Habib Chowdhury]

Created attachment 109717 [details]
s/srollBy/scrollTo/

Changed to use scrollTo instead of scrollBy.

[Sadrul Habib Chowdhury]

Comment on attachment 109717 [details]
s/srollBy/scrollTo/

This may still need some changes. I will test some more.

[Sadrul Habib Chowdhury]

Created attachment 109723 [details]
setScrollPosition

ScrollView::scrollTo has a NOTE saying it should only be called by the overriden setScrollOffset from ScrollableArea, and FrameView::scrollToAnchor has a comment saying it can trigger setScrollPosition. So I have changed here to use setScrollPosition instead of scrollTo/scrollBy. Does this look OK?

[WebKit Review Bot]

Comment on attachment 109723 [details]
setScrollPosition

Clearing flags on attachment: 109723

Committed r96672: <http://trac.webkit.org/changeset/96672>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.