Bug 193494 – [iOS] Content offset jumps erratically when autoscrolling near scroll view content inset areas

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Wenson Hsieh]

<rdar://problem/46859627>

[Wenson Hsieh]

Created attachment 359379 [details]
Patch

[Tim Horton]

Comment on attachment 359379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359379&action=review

> Source/WebKit/ChangeLog:26
> +        We don't care about binary compatibility with iOS 10 and below anymore, so we should change these >= iOS 11
> +        target checks to simply `PLATFORM(IOS)`.

This is about source compatibility, not bincompat, right?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2979
> +    auto scrollViewOriginIncludingInset = UIEdgeInsetsInsetRect([_scrollView bounds], computedContentInsetUnadjustedForKeyboard).origin;

Should we stash aside [_scrollView bounds] and contentSize to avoid all these messages?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2983
> +    if (scrollViewInsets.left > 0 && scrollViewOriginIncludingInset.x < 0)

This all could probably use a comment. Also I am looking at UIGeometry headers longingly wishing there were a better way to write all this.

> Tools/WebKitTestRunner/TestController.cpp:1270
> +        else if (key == "topContentInset")
> +            testOptions.topContentInset = std::stod(value);

Please don't propagate the silly "topContentInset" thing. This will be confusing when getting rid of the Mac notion.

I wonder if we can actually put dots here? like contentInset.top in the key name?

[Tim Horton]

Comment on attachment 359379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359379&action=review

> Source/WebCore/rendering/RenderLayer.cpp:2557
> +            // to handle the case where the rect to reveal is outside the content, but within the inset area.

I feel like these words aren't quite right? the rect to reveal can be inside the content and still be affected by this.

[Tim Horton]

Probably want smfr to peek too.

[Simon Fraser (smfr)]

Comment on attachment 359379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359379&action=review

> Source/WebCore/rendering/RenderLayer.cpp:2558
> +            auto& unobscuredInsets = page().unobscuredContentInsets();

Rather than fetching insets form Page and getting unobscuredContentRect() and doing math, I'd prefer that FrameView have a single function to returned the rect expanded by insets.

[Wenson Hsieh]

Comment on attachment 359379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359379&action=review

>> Source/WebKit/ChangeLog:26
>> +        target checks to simply `PLATFORM(IOS)`.
> 
> This is about source compatibility, not bincompat, right?

Oops, right. Fixed!

>> Source/WebCore/rendering/RenderLayer.cpp:2557
>> +            // to handle the case where the rect to reveal is outside the content, but within the inset area.
> 
> I feel like these words aren't quite right? the rect to reveal can be inside the content and still be affected by this.

Indeed. This is really about allowing this code to scroll into (or keep the scroll position) inside a content inset area, instead of always staying in the content rect. I tweaked the comment here to better reflect this.

>> Source/WebCore/rendering/RenderLayer.cpp:2558
>> +            auto& unobscuredInsets = page().unobscuredContentInsets();
> 
> Rather than fetching insets form Page and getting unobscuredContentRect() and doing math, I'd prefer that FrameView have a single function to returned the rect expanded by insets.

Good call. I'll abstract this into a helper method (perhaps FrameView::unobscuredContentRectIncludingInsets?)

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2979
>> +    auto scrollViewOriginIncludingInset = UIEdgeInsetsInsetRect([_scrollView bounds], computedContentInsetUnadjustedForKeyboard).origin;
> 
> Should we stash aside [_scrollView bounds] and contentSize to avoid all these messages?

Yes! Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2983
>> +    if (scrollViewInsets.left > 0 && scrollViewOriginIncludingInset.x < 0)
> 
> This all could probably use a comment. Also I am looking at UIGeometry headers longingly wishing there were a better way to write all this.

Yeah... :/

I added a comment here describing the logic.

>> Tools/WebKitTestRunner/TestController.cpp:1270
>> +            testOptions.topContentInset = std::stod(value);
> 
> Please don't propagate the silly "topContentInset" thing. This will be confusing when getting rid of the Mac notion.
> 
> I wonder if we can actually put dots here? like contentInset.top in the key name?

👍 I'll use "contentInset.top" and `contentInsetTop`

[Wenson Hsieh]

Simon also suggested that I rename "unobscuredContentInsets" to just "contentInsets", and also call the new helper something like "unobscuredContentRectExpandedByContentInsets".

[Wenson Hsieh]

Created attachment 359437 [details]
Patch v2

[Simon Fraser (smfr)]

Comment on attachment 359437 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=359437&action=review

> Source/WebCore/rendering/RenderLayer.cpp:2555
> +            // Even though content insets do not represent content, we consider them as part of the visible rect
> +            // to allow ourselves to scroll into content insets.

I think it would be better to just remove this comment.

[Wenson Hsieh]

Created attachment 359441 [details]
Patch for landing

[Wenson Hsieh]

Created attachment 359442 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 359442 [details]
Patch for landing

Clearing flags on attachment: 359442

Committed r240139: <https://trac.webkit.org/changeset/240139>

[Mohan]

*** Bug 209821 has been marked as a duplicate of this bug. ***