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-
Patch (13.96 KB, patch)
2021-07-21 17:26 PDT, Megan Gardner
ews-feeder: commit-queue-
Patch (13.89 KB, patch)
2021-07-21 17:32 PDT, Megan Gardner
no flags
Patch (14.31 KB, patch)
2021-07-21 19:16 PDT, Megan Gardner
no flags
Patch (14.32 KB, patch)
2021-07-21 22:42 PDT, Megan Gardner
no flags
Patch (14.30 KB, patch)
2021-07-22 08:04 PDT, Megan Gardner
no flags
Patch (14.30 KB, patch)
2021-07-22 09:06 PDT, Megan Gardner
no flags
Patch for landing (14.33 KB, patch)
2021-07-22 09:24 PDT, Megan Gardner
no flags
Megan Gardner
Comment 1 2021-07-21 17:18:35 PDT
Megan Gardner
Comment 2 2021-07-21 17:26:59 PDT
Megan Gardner
Comment 3 2021-07-21 17:32:34 PDT
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
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
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
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
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
Note You need to log in before you can comment on or make changes to this bug.