Summary: | Relax ScrollView::adjustScrollPositionWithinRange in case constrainsScrollingToContentEdge is false | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antonio Gomes <tonikitoo> | ||||||
Component: | Platform | Assignee: | Antonio Gomes <tonikitoo> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | andersca, kenneth, manyoso, simon.fraser | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 73144 | ||||||||
Attachments: |
|
Description
Antonio Gomes
2012-04-17 11:56:28 PDT
*** Bug 84176 has been marked as a duplicate of this bug. *** 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. (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). 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 } .... Created attachment 137582 [details]
(wrong patch, please ignore) patch
Patch for discussion.
Call sites of ScrollView::adjustScrollPositionWithinRange from ::setScrollPosition and ::updateScrollbars.
Created attachment 137708 [details] (committed r114770, r=atreat) patch - the right patch this time <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 on attachment 137708 [details] (committed r114770, r=atreat) patch - the right patch this time lgtm and tested against mac port with no regression so r+ Comment on attachment 137708 [details] (committed r114770, r=atreat) patch - the right patch this time Committed http://trac.webkit.org/changeset/114770 |