Bug 228172 – Avoid Quick Note overlay when scrolling to show a highlight

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Avoid Quick Note overlay when scrolling to show a highlight

[Megan Gardner]

Created attachment 433977 [details]
Patch

[Megan Gardner]

Created attachment 433978 [details]
Patch

[Megan Gardner]

Created attachment 433979 [details]
Patch

[Tim Horton]

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 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 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]

Created attachment 433982 [details]
Patch

[Tim Horton]

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]

Created attachment 433992 [details]
Patch

[Tim Horton]

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 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]

Created attachment 434010 [details]
Patch

[Wenson Hsieh]

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 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]

Created attachment 434012 [details]
Patch

[Wenson Hsieh]

Comment on attachment 434012 [details]
Patch

r=me+thorton

[Megan Gardner]

Created attachment 434013 [details]
Patch for landing

[EWS]

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]

<rdar://problem/80968199>