Summary: | [iOS WK2] Page jumps when rubber-banding on azuremagazine.com | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||
Component: | New Bugs | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, bunhere, cdumez, commit-queue, gyuyoung.kim, ian, sergio, simon.fraser | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2014-06-23 21:33:32 PDT
Created attachment 233679 [details]
Patch
Comment on attachment 233679 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233679&action=review > Source/WebKit2/UIProcess/ios/WKScrollView.mm:224 > + CGSize maxContentOffset = CGSizeMake(contentSize.width - bounds.size.width, contentSize.height - bounds.size.height); I am a little confused by this. Why does this max offset ignore the contentInsets? > Source/WebKit2/UIProcess/ios/WKScrollView.mm:229 > + adjustedOffset.y = maxContentOffset.width + edgeInsets.right + rubberbandAmount.width; .y here instead of .x > Source/WebKit2/UIProcess/ios/WKScrollView.mm:236 > + [self setContentOffset:adjustedOffset]; Does UIScrollView just resume rubberbanding from that offset? (In reply to comment #2) > (From update of attachment 233679 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=233679&action=review > > > Source/WebKit2/UIProcess/ios/WKScrollView.mm:224 > > + CGSize maxContentOffset = CGSizeMake(contentSize.width - bounds.size.width, contentSize.height - bounds.size.height); > > I am a little confused by this. Why does this max offset ignore the contentInsets? I just took edge insets into account lower down, but can move them up here. > > > Source/WebKit2/UIProcess/ios/WKScrollView.mm:229 > > + adjustedOffset.y = maxContentOffset.width + edgeInsets.right + rubberbandAmount.width; > > .y here instead of .x Thanks. > > Source/WebKit2/UIProcess/ios/WKScrollView.mm:236 > > + [self setContentOffset:adjustedOffset]; > > Does UIScrollView just resume rubberbanding from that offset? It seems to, yes. Created attachment 233738 [details]
Patch
Comment on attachment 233738 [details]
Patch
Not right when the page gets longer.
Created attachment 233773 [details]
Patch
Attachment 233773 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/ios/WKScrollView.mm:235: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKScrollView.mm:245: Multi line control clauses should use braces. [whitespace/braces] [4]
Total errors found: 2 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 233773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233773&action=review I think that is okay for growing content. At the next timer update, the new BouncingDecelerationOffset will be withint bounds. > Source/WebKit2/UIProcess/ios/WKScrollView.h:37 > +- (void)_setContentSizePreservingContentOffsetDuringRubberband:(CGSize)contentSize; > + Not sure you need the blank lines below and above, I know nothing about Obj-C coding style :) > Source/WebKit2/UIProcess/ios/WKScrollView.mm:234 > + if (rubberbandAmount.width < 0) // Left > + adjustedOffset.x = -edgeInsets.left + rubberbandAmount.width; I think you should check this in the case where the page size change when it is scaled smaller than the viewport minimum size. It may work as expected because in _currentRubberbandAmount, you check for top/left before bottom/right, but better check. > Source/WebKit2/UIProcess/ios/WKScrollView.mm:260 > + if (CGSizeEqualToSize(currentContentSize, CGSizeZero) || CGSizeEqualToSize(currentContentSize, contentSize)) { You could early return on CGSizeEqualToSize(currentContentSize, contentSize). Created attachment 233840 [details]
Patch
|