Fix FrameView::scrollElementToRect to take already scrolled amount into consideration.
Created attachment 109415 [details] patch I updated the existing layout test to scroll first, so that this fix is covered by the test.
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.
(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.
(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).
Comment on attachment 109415 [details] patch The really should be using FrameView::scrollTo, rather than scrollBy so that it's no longer relative.
(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.
> 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.
(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.
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.
(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
Created attachment 109717 [details] s/srollBy/scrollTo/ Changed to use scrollTo instead of scrollBy.
Comment on attachment 109717 [details] s/srollBy/scrollTo/ This may still need some changes. I will test some more.
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?
Comment on attachment 109723 [details] setScrollPosition Clearing flags on attachment: 109723 Committed r96672: <http://trac.webkit.org/changeset/96672>
All reviewed patches have been landed. Closing bug.