[iOS WK2] Page jumps when rubber-banding on azuremagazine.com
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
https://trac.webkit.org/r170463