Summary: | Avoid Quick Note overlay when scrolling to show a highlight | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Megan Gardner <megan_gardner> | ||||||||||||||||||
Component: | New Bugs | Assignee: | Megan Gardner <megan_gardner> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | ews-watchlist, mifenton, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
Megan Gardner
2021-07-21 16:54:51 PDT
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]. |