<rdar://problem/46859627>
Created attachment 359379 [details] Patch
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?
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.
Probably want smfr to peek too.
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.
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`
Simon also suggested that I rename "unobscuredContentInsets" to just "contentInsets", and also call the new helper something like "unobscuredContentRectExpandedByContentInsets".
Created attachment 359437 [details] Patch v2
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.
Created attachment 359441 [details] Patch for landing
Created attachment 359442 [details] Patch for landing
Comment on attachment 359442 [details] Patch for landing Clearing flags on attachment: 359442 Committed r240139: <https://trac.webkit.org/changeset/240139>
*** Bug 209821 has been marked as a duplicate of this bug. ***