WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
<
rdar://problem/46859627
>
Attachments
Patch
(35.55 KB, patch)
2019-01-17 08:22 PST
,
Wenson Hsieh
simon.fraser
: review-
Details
Formatted Diff
Diff
Patch v2
(39.93 KB, patch)
2019-01-17 17:52 PST
,
Wenson Hsieh
simon.fraser
: review+
Details
Formatted Diff
Diff
Patch for landing
(39.80 KB, patch)
2019-01-17 19:32 PST
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Patch for landing
(39.74 KB, patch)
2019-01-17 19:36 PST
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Wenson Hsieh
Comment 1
2019-01-17 08:22:26 PST
Created
attachment 359379
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug