Bug 134238 – [iOS WK2] Page jumps when rubber-banding on azuremagazine.com

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

[iOS WK2] Page jumps when rubber-banding on azuremagazine.com

[Simon Fraser (smfr)]

Created attachment 233679 [details]
Patch

[Benjamin Poulain]

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?

[Simon Fraser (smfr)]

(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.

[Simon Fraser (smfr)]

Created attachment 233738 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 233738 [details]
Patch

Not right when the page gets longer.

[Simon Fraser (smfr)]

Created attachment 233773 [details]
Patch

[WebKit Commit Bot]

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.

[Benjamin Poulain]

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).

[Simon Fraser (smfr)]

Created attachment 233840 [details]
Patch

[Simon Fraser (smfr)]

https://trac.webkit.org/r170463