Bug 84178 - Relax ScrollView::adjustScrollPositionWithinRange in case constrainsScrollingToContentEdge is false
: Relax ScrollView::adjustScrollPositionWithinRange in case constrainsScrolling...
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 73144
  Show dependency treegraph
 
Reported: 2012-04-17 11:56 PST by
Modified: 2012-04-20 12:33 PST (History)


Attachments
(wrong patch, please ignore) patch (4.13 KB, patch)
2012-04-17 12:23 PST, Antonio Gomes
no flags Review Patch | Details | Formatted Diff | Diff
(committed r114770, r=atreat) patch - the right patch this time (2.91 KB, patch)
2012-04-18 09:37 PST, Antonio Gomes
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-17 11:56:28 PST
Right now, when a port has delegatesScrolling set to FALSE, and constrainsScrollingToContentEdge set to FALSE (i.e. it accepts overscrolled position), calling ScrollView::setScrollPosition with an overscrolled position still gets the position clamped to the content size edges.

We could relax ::adjustScrollPositionWithinRange in that sense.
------- Comment #1 From 2012-04-17 11:56:44 PST -------
*** Bug 84176 has been marked as a duplicate of this bug. ***
------- Comment #2 From 2012-04-17 11:58:45 PST -------
I think doing so would break a lot of assumptions inside WebCore. I tried doing something like this in ScrollView::updateScrollbars and it did not work well.
------- Comment #3 From 2012-04-17 12:12:06 PST -------
(In reply to comment #2)
> I think doing so would break a lot of assumptions inside WebCore. I tried doing something like this in ScrollView::updateScrollbars and it did not work well.

Would you remember what kind of assumptions? We run with this since a while without problems...

If we do not have such a "relaxation" if user over scrolls a webpage, say to (0, -20) webcore would be tracking a scroll position of (0, 0).
------- Comment #4 From 2012-04-17 12:20:47 PST -------
Note that ScrollView::setScrollOffset does something similar already:

340 void ScrollView::setScrollOffset(const IntPoint& offset)
341 {                                                                          342     int horizontalOffset = offset.x();
343     int verticalOffset = offset.y();
344     if (constrainsScrollingToContentEdge()) {
345         horizontalOffset = max(min(horizontalOffset, contentsWidth() - visibleWidth()), 0);
346         verticalOffset = max(min(verticalOffset, contentsHeight() - visibleHeight()), 0); 
347     }        
....
------- Comment #5 From 2012-04-17 12:23:42 PST -------
Created an attachment (id=137582) [details]
patch

Patch for discussion.

Call  sites of ScrollView::adjustScrollPositionWithinRange from ::setScrollPosition and ::updateScrollbars.
------- Comment #6 From 2012-04-18 09:37:38 PST -------
Created an attachment (id=137708) [details]
patch  (the right patch this time)
------- Comment #7 From 2012-04-19 22:15:31 PST -------
<tonikitoo> andersca, sir, would you have some minutes to talk about? https://bugs.webkit.org/show_bug.cgi?id=84178
<tonikitoo> (shame we can not talk in person during the summit :( )
<andersca>  tonikitoo: yes, I think I was wrong about my assumption
<andersca>  tonikitoo: if you have a Mac you can test if it breaks things by going to a page with a vertical scrollbar, scroll to the bottom
<andersca>  tonikitoo: then hit command-f to show the find banner and escape to hide it
<tonikitoo> scrollview::setscrollposition calls it I think
<andersca>  tonikitoo: and make sure that the behavior is the same even with your patch
...
<tonikitoo> andersca, just to let you know: I got it tested on a safari build with and without the patch, and behaviour looks the same
<andersca>  tonikitoo: excellent
------- Comment #8 From 2012-04-20 11:43:33 PST -------
(From update of attachment 137708 [details])
lgtm and tested against mac port with no regression so r+
------- Comment #9 From 2012-04-20 12:32:40 PST -------
(From update of attachment 137708 [details])
Committed http://trac.webkit.org/changeset/114770