RESOLVED FIXED 193494
[iOS] Content offset jumps erratically when autoscrolling near scroll view content inset areas
https://bugs.webkit.org/show_bug.cgi?id=193494
Summary [iOS] Content offset jumps erratically when autoscrolling near scroll view co...
Wenson Hsieh
Reported 2019-01-16 08:01:14 PST
Attachments
Patch (35.55 KB, patch)
2019-01-17 08:22 PST, Wenson Hsieh
simon.fraser: review-
Patch v2 (39.93 KB, patch)
2019-01-17 17:52 PST, Wenson Hsieh
simon.fraser: review+
Patch for landing (39.80 KB, patch)
2019-01-17 19:32 PST, Wenson Hsieh
no flags
Patch for landing (39.74 KB, patch)
2019-01-17 19:36 PST, Wenson Hsieh
no flags
Wenson Hsieh
Comment 1 2019-01-17 08:22:26 PST
Tim Horton
Comment 2 2019-01-17 13:45:08 PST
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 3 2019-01-17 13:45:54 PST
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
Comment 4 2019-01-17 13:46:09 PST
Probably want smfr to peek too.
Simon Fraser (smfr)
Comment 5 2019-01-17 14:31:18 PST
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 6 2019-01-17 14:54:43 PST
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
Comment 7 2019-01-17 16:11:41 PST
Simon also suggested that I rename "unobscuredContentInsets" to just "contentInsets", and also call the new helper something like "unobscuredContentRectExpandedByContentInsets".
Wenson Hsieh
Comment 8 2019-01-17 17:52:29 PST
Created attachment 359437 [details] Patch v2
Simon Fraser (smfr)
Comment 9 2019-01-17 17:59:55 PST
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
Comment 10 2019-01-17 19:32:11 PST
Created attachment 359441 [details] Patch for landing
Wenson Hsieh
Comment 11 2019-01-17 19:36:41 PST
Created attachment 359442 [details] Patch for landing
WebKit Commit Bot
Comment 12 2019-01-17 20:15:31 PST
Comment on attachment 359442 [details] Patch for landing Clearing flags on attachment: 359442 Committed r240139: <https://trac.webkit.org/changeset/240139>
Mohan
Comment 13 2020-04-01 06:00:30 PDT
*** Bug 209821 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.