WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
228172
Avoid Quick Note overlay when scrolling to show a highlight
https://bugs.webkit.org/show_bug.cgi?id=228172
Summary
Avoid Quick Note overlay when scrolling to show a highlight
Megan Gardner
Reported
2021-07-21 16:54:51 PDT
Avoid Quick Note overlay when scrolling to show a highlight
Attachments
Patch
(13.91 KB, patch)
2021-07-21 17:18 PDT
,
Megan Gardner
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(13.96 KB, patch)
2021-07-21 17:26 PDT
,
Megan Gardner
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(13.89 KB, patch)
2021-07-21 17:32 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(14.31 KB, patch)
2021-07-21 19:16 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(14.32 KB, patch)
2021-07-21 22:42 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(14.30 KB, patch)
2021-07-22 08:04 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(14.30 KB, patch)
2021-07-22 09:06 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch for landing
(14.33 KB, patch)
2021-07-22 09:24 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Megan Gardner
Comment 1
2021-07-21 17:18:35 PDT
Created
attachment 433977
[details]
Patch
Megan Gardner
Comment 2
2021-07-21 17:26:59 PDT
Created
attachment 433978
[details]
Patch
Megan Gardner
Comment 3
2021-07-21 17:32:34 PDT
Created
attachment 433979
[details]
Patch
Tim Horton
Comment 4
2021-07-21 17:37:44 PDT
Comment on
attachment 433977
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=433977&action=review
> Source/WebCore/Modules/highlight/AppHighlightStorage.cpp:274 > + TemporarySelectionChange selectionChange(*strongDocument, { range.value() }, { TemporarySelectionOption::DelegateMainFrameScroll, TemporarySelectionOption::SmoothScroll, TemporarySelectionOption::ScrollToFullSelection });
Wenson is going to leave a note about this name, and I think we should fix all of them in a followup.
> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1156 > +- (float)adjustScrollRect:(WebCore::FloatRect)targetRect
This needs an underscore, and probably more words (because this is on WKWebView which is API)
> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1158 > + WebCore::FloatRect overlayRect = [self convertRect:_page->appHighlightsNoteOverlayRect() fromCoordinateSpace:[_scrollView window].screen.coordinateSpace];
Probably just self.window.screen.coordinateSpace, no need to involve the poor innocent scrollview.
> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1159 > +
What is the overlay rect if the PIP is off screen? Should we just bail if it's not shown?
> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1160 > + overlayRect.expand(5, 5);
Magic numbers should be factored out.
> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1165 > + float topGap = overlayRect.y() - [self bounds].origin.y;
More dots, as always. (bounds and center)
> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1221 > + float additionalOffset = [self adjustScrollRect:convertedStartRect];
weird extraneous space
> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:606 > +CGRect WebPageProxy::appHighlightsNoteOverlayRect()
You could totally just get rid of the word "Note" here. It is the overlay for app highlights, regardless of the content
Wenson Hsieh
Comment 5
2021-07-21 17:41:09 PDT
Comment on
attachment 433979
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=433979&action=review
> Source/WebCore/editing/Editor.h:126 > + ScrollToFullSelection = 1 << 6,
Perhaps `RevealSelectionBounds` instead of `ScrollToFullSelection`?
> Source/WebCore/editing/FrameSelection.h:128 > + ScrollToFullSelection = 1 << 11
Ditto - perhaps `RevealSelectionBounds` instead of `ScrollToFullSelection`?
> Source/WebCore/editing/FrameSelection.h:267 > + void updateAndRevealSelection(const AXTextStateChangeIntent&, ScrollBehavior = ScrollBehavior::Instant, RevealExtentOption = RevealExtentOption::RevealExtent);
This isn't something we should try and tackle in this same patch, but we should consider renaming `DoNotRevealExtent` to something like `RevealSelectionBounds` to make it clear that one option is for revealing to the extent, while the other option is for revealing the entire bounds of the selection.
Wenson Hsieh
Comment 6
2021-07-21 18:45:47 PDT
Comment on
attachment 433977
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=433977&action=review
>> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1159 >> + > > What is the overlay rect if the PIP is off screen? Should we just bail if it's not shown?
From code inspection, it seems like it'll be `CGRectNull`.
Megan Gardner
Comment 7
2021-07-21 19:16:11 PDT
Created
attachment 433982
[details]
Patch
Tim Horton
Comment 8
2021-07-21 19:19:34 PDT
Comment on
attachment 433982
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=433982&action=review
> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1185 > +#endif > + return 0;
In the ENABLE(APP_HIGHLIGHTS) case you end up having 2 returns in a row, which is pretty weird
Megan Gardner
Comment 9
2021-07-21 22:42:59 PDT
Created
attachment 433992
[details]
Patch
Tim Horton
Comment 10
2021-07-21 23:01:40 PDT
Comment on
attachment 433992
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=433992&action=review
stylebot complaint is /mystifying/
> Source/WebCore/ChangeLog:3 > + Avoid Quick Note overlay when scrolling to show a highlight
This seems straightforwardly API-testable (swizzle the Synapse observer, restore+scroll, see where you end up), you should write some tests.
> Source/WebCore/editing/FrameSelection.cpp:2334 > + auto visibleSelectionRect = IntRect();
This is a funny way to write `IntRect visibleSelectionRect;`
> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1173 > + float midScreen = [self center].y;
dots!
Tim Horton
Comment 11
2021-07-21 23:02:40 PDT
Comment on
attachment 433992
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=433992&action=review
> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1162 > + if (overlayRect == CGRectNull)
CGRectIsNull
Megan Gardner
Comment 12
2021-07-22 08:04:30 PDT
Created
attachment 434010
[details]
Patch
Wenson Hsieh
Comment 13
2021-07-22 08:07:13 PDT
Comment on
attachment 434010
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=434010&action=review
> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1162 > + if (CGRectIsNull(CGRectNull))
You should probably use the `overlayRect` here.
Megan Gardner
Comment 14
2021-07-22 09:05:48 PDT
Comment on
attachment 434010
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=434010&action=review
>> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:1162 >> + if (CGRectIsNull(CGRectNull)) > > You should probably use the `overlayRect` here.
Thanks for catching that, this is what I get for trying fix things really quick before a meeting...
Megan Gardner
Comment 15
2021-07-22 09:06:02 PDT
Created
attachment 434012
[details]
Patch
Wenson Hsieh
Comment 16
2021-07-22 09:21:04 PDT
Comment on
attachment 434012
[details]
Patch r=me+thorton
Megan Gardner
Comment 17
2021-07-22 09:24:29 PDT
Created
attachment 434013
[details]
Patch for landing
EWS
Comment 18
2021-07-22 10:07:28 PDT
Committed
r280178
(
239872@main
): <
https://commits.webkit.org/239872@main
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 434013
[details]
.
Radar WebKit Bug Importer
Comment 19
2021-07-22 10:08:16 PDT
<
rdar://problem/80968199
>
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