Avoid Quick Note overlay when scrolling to show a highlight
Created attachment 433977 [details] Patch
Created attachment 433978 [details] Patch
Created attachment 433979 [details] Patch
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
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.
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`.
Created attachment 433982 [details] Patch
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
Created attachment 433992 [details] Patch
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!
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
Created attachment 434010 [details] Patch
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.
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...
Created attachment 434012 [details] Patch
Comment on attachment 434012 [details] Patch r=me+thorton
Created attachment 434013 [details] Patch for landing
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].
<rdar://problem/80968199>