Bug 224773 - Support scrolling to a selected AppHighlight
Summary: Support scrolling to a selected AppHighlight
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-19 11:26 PDT by Megan Gardner
Modified: 2021-04-21 10:56 PDT (History)
5 users (show)

See Also:


Attachments
Patch (26.48 KB, patch)
2021-04-19 11:34 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (28.93 KB, patch)
2021-04-19 17:37 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (28.06 KB, patch)
2021-04-20 17:14 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (22.14 KB, patch)
2021-04-20 17:47 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (24.34 KB, patch)
2021-04-20 21:23 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (24.45 KB, patch)
2021-04-20 22:09 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (24.44 KB, patch)
2021-04-20 23:14 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2021-04-19 11:26:35 PDT
Support scrolling to a selected AppHighlight
Comment 1 Megan Gardner 2021-04-19 11:34:50 PDT
Created attachment 426450 [details]
Patch
Comment 2 Tim Horton 2021-04-19 11:45:30 PDT
Comment on attachment 426450 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426450&action=review

> Source/WebCore/Modules/highlight/AppHighlightStorage.h:71
> +    Optional<AppHighlightRangeData> m_unrestoreScrollHighlight;

"unrestore"? unrestored, maybe, like the one above?

> Source/WebCore/page/Page.cpp:1594
> +#if ENABLE(APP_HIGHLIGHTS)

Newline above

> Source/WebCore/page/Page.cpp:1602
> +        if (appHighlightStorage->hasUnrestoredHighlights() && MonotonicTime::now() - appHighlightStorage->lastRangeSearchTime() > 1_s) {

Why is all this code duplicated with that down below? Something is wrong with the factoring here. (also maybe the logic? what's up with this one second comparison here, after the one second timer?)

> Source/WebKit/UIProcess/WebPageProxy.h:1886
> +    void restoreAppHighlightsAndScrollToIndex(const Vector<Ref<WebKit::SharedMemory>>& highlights, const int64_t index);

Why a signed index?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7498
> +            document->appHighlightStorage().restoreAndScrollToAppHighlight(SharedBuffer::create(static_cast<const char*>(sharedMemory->data()), sharedMemory->size()), ScrollToHighlight::Yes);

This will be a lot less duplicative if you shove the comparison into the last argument:

document->appHighlightStorage().restoreAndScrollToAppHighlight(SharedBuffer::create(static_cast<const char*>(sharedMemory->data()), sharedMemory->size()), i == index ? ScrollToHighlight::Yes : ScrollToHighlight::No);
Comment 3 Wenson Hsieh 2021-04-19 11:46:29 PDT
Comment on attachment 426450 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426450&action=review

> Source/WebCore/Modules/highlight/AppHighlightStorage.h:67
> +    bool attemptHighlightRestoreAndScroll(AppHighlightRangeData&, ScrollToHighlight);

Nit - maybe `attemptToRestoreHighlightAndScroll`?

> Source/WebCore/Modules/highlight/AppHighlightStorage.h:71
> +    Optional<AppHighlightRangeData> m_unrestoreScrollHighlight;

Nit - `m_unrestoredScrollHighlight`

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2184
> +- (void)_restoreAndScrollToAppHighlight:(NSData *)highlight
> +{
> +#if ENABLE(APP_HIGHLIGHTS)
> +    Vector<Ref<WebKit::SharedMemory>> buffers;
> +    
> +    auto sharedMemory = WebKit::SharedMemory::allocate(highlight.length);
> +    if (sharedMemory) {
> +        [highlight getBytes:sharedMemory->data() length:highlight.length];
> +        buffers.append(*sharedMemory);
> +    }
> +    _page->restoreAppHighlightsAndScrollToIndex(buffers, 0);

Nit - maybe some of this code can be shared with `-_restoreAppHighlights:` via a static helper function?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7500
> +        if (i == index)
> +            document->appHighlightStorage().restoreAndScrollToAppHighlight(SharedBuffer::create(static_cast<const char*>(sharedMemory->data()), sharedMemory->size()), ScrollToHighlight::Yes);
> +        else
> +            document->appHighlightStorage().restoreAndScrollToAppHighlight(SharedBuffer::create(static_cast<const char*>(sharedMemory->data()), sharedMemory->size()), ScrollToHighlight::No);

Nit - I think you can avoid duplication here as well by making the `ScrollToHighlight` argument a local variable.
Comment 4 Wenson Hsieh 2021-04-19 12:12:13 PDT
Comment on attachment 426450 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426450&action=review

> Source/WebCore/Modules/highlight/AppHighlightStorage.cpp:266
> +        TemporarySelectionChange selectionChange(*strongDocument, { range.value() }, temporarySelectionOptions);

Does this option work in the case where there was already a selection in the DOM prior to highlight restoration?
Comment 5 Megan Gardner 2021-04-19 17:37:02 PDT
Created attachment 426498 [details]
Patch
Comment 6 Tim Horton 2021-04-20 13:50:12 PDT
Comment on attachment 426498 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426498&action=review

> Source/WebCore/Modules/highlight/AppHighlightStorage.h:29
> +

No newline here.

> Source/WebCore/page/Page.cpp:1623
> +#if ENABLE(APP_HIGHLIGHTS)

Oof

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2157
> +static void restoreHighlight(Vector<Ref<WebKit::SharedMemory>> &buffers, NSData* highlight)

This does no restoration, just does type conversion and adds it to a vector.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7492
> +    unsigned i = 0;

I think this type and the inner type of index should match, though I don't know which is right.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7501
> +            document->appHighlightStorage().restoreAndScrollToAppHighlight(SharedBuffer::create(static_cast<const char*>(sharedMemory->data()), sharedMemory->size()), ScrollToHighlight::Yes);
> +        else
> +            document->appHighlightStorage().restoreAndScrollToAppHighlight(SharedBuffer::create(static_cast<const char*>(sharedMemory->data()), sharedMemory->size()), ScrollToHighlight::No);

Multiple review comments about this code above were not applied. Intentional?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:95
> +    [webViewCreate synchronouslyLoadHTMLString:@"<br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br>Test"];

<div style='height: 1000px'></div>? Or just use lots-of-text.html like all the other tests.
Comment 7 Wenson Hsieh 2021-04-20 14:14:05 PDT
Comment on attachment 426498 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426498&action=review

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2157
>> +static void restoreHighlight(Vector<Ref<WebKit::SharedMemory>> &buffers, NSData* highlight)
> 
> This does no restoration, just does type conversion and adds it to a vector.

Also, minor nit - `&` and `*` are on the wrong sides. Perhaps a better helper method would be something along the lines of:

static Vector<Ref<WebKit::SharedMemory>> copyHighlightDataToSharedMemory(NSArray<NSData *> *highlights)
{
    …
}
Comment 8 Megan Gardner 2021-04-20 17:14:07 PDT
Created attachment 426621 [details]
Patch
Comment 9 Megan Gardner 2021-04-20 17:47:23 PDT
Created attachment 426625 [details]
Patch
Comment 10 Wenson Hsieh 2021-04-20 18:03:07 PDT
Comment on attachment 426625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426625&action=review

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:110
> +        EXPECT_NE(0, [[webViewRestore objectByEvaluatingJavaScript:@"pageYOffset"] floatValue]);

Would we actually expect to scroll in this case? It seems like we're restoring highlights in a web view that is not scrollable, since the web view we're restoring highlights in just contains "Test".
Comment 11 Megan Gardner 2021-04-20 20:10:36 PDT
Comment on attachment 426625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426625&action=review

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:110
>> +        EXPECT_NE(0, [[webViewRestore objectByEvaluatingJavaScript:@"pageYOffset"] floatValue]);
> 
> Would we actually expect to scroll in this case? It seems like we're restoring highlights in a web view that is not scrollable, since the web view we're restoring highlights in just contains "Test".

I used to have a bunch of <br>s but Tim said to just have a tall div instead.
Comment 12 Tim Horton 2021-04-20 20:18:46 PDT
(In reply to Megan Gardner from comment #11)
> Comment on attachment 426625 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426625&action=review
> 
> >> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:110
> >> +        EXPECT_NE(0, [[webViewRestore objectByEvaluatingJavaScript:@"pageYOffset"] floatValue]);
> > 
> > Would we actually expect to scroll in this case? It seems like we're restoring highlights in a web view that is not scrollable, since the web view we're restoring highlights in just contains "Test".
> 
> I used to have a bunch of <br>s but Tim said to just have a tall div instead.

I did, but:

1) you put the text inside the div; I think you wanted to put it after, and 
2) your "restore" webview only has this: [webViewRestore synchronouslyLoadHTMLString:@"Test"];
Comment 13 Wenson Hsieh 2021-04-20 20:19:06 PDT
Comment on attachment 426625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426625&action=review

>>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAppHighlights.mm:110
>>> +        EXPECT_NE(0, [[webViewRestore objectByEvaluatingJavaScript:@"pageYOffset"] floatValue]);
>> 
>> Would we actually expect to scroll in this case? It seems like we're restoring highlights in a web view that is not scrollable, since the web view we're restoring highlights in just contains "Test".
> 
> I used to have a bunch of <br>s but Tim said to just have a tall div instead.

I do agree that having a tall div element is a bit cleaner than lots of br elements. But that was on the WKWebView where we created the highlight, right? When restoring the highlight, it looks like we're restoring the highlight to an unscrollable web page…(I suppose it's not really clear to me why we expect `pageYOffset != 0` after restoration).

Also, it looks like the test is calling `-_restoreAppHighlights:` instead of `-_restoreAndScrollToAppHighlight:`. Does the former also trigger scrolling, or is it just the latter?
Comment 14 Megan Gardner 2021-04-20 21:23:38 PDT
Created attachment 426640 [details]
Patch
Comment 15 Wenson Hsieh 2021-04-20 21:37:07 PDT
Comment on attachment 426640 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426640&action=review

Looking much better! A couple of minor comments.

> Source/WebCore/Modules/highlight/AppHighlightStorage.h:63
>      bool hasUnrestoredHighlights() const { return m_unrestoredHighlights.size(); }

Do we need to update this implementation to `return m_unrestoredScrollHighlight || m_unrestoredHighlights.size();`? It looks like in the case where we have an unrestored highlight that we need to scroll to, the highlight won't end up being stored in `m_unrestoredHighlights`, so this will return `false`.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2157
> +static void convertAndAddHighlight(Vector<Ref<WebKit::SharedMemory>> &buffers, NSData* highlight)

Nit - the `&` and `*` are still on the wrong side (https://bugs.webkit.org/show_bug.cgi?id=224773#c7).
Comment 16 Tim Horton 2021-04-20 21:37:34 PDT
Comment on attachment 426640 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426640&action=review

> Source/WebCore/Modules/highlight/AppHighlightStorage.h:66
> +    bool attemptToRestoreHighlightAndScroll(AppHighlightRangeData&, ScrollToHighlight);

Newline below here, before the members.

> Source/WebCore/page/Page.cpp:1623
> -#if ENABLE(APP_HIGHLIGHT)
> +#if ENABLE(APP_HIGHLIGHTS)

Should make sure that there's a test that covers this code (maybe in another patch)

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2157
> +static void convertAndAddHighlight(Vector<Ref<WebKit::SharedMemory>> &buffers, NSData* highlight)

star's on the wrong side
Comment 17 Megan Gardner 2021-04-20 22:09:13 PDT
Created attachment 426647 [details]
Patch
Comment 18 Tim Horton 2021-04-20 22:21:47 PDT
Comment on attachment 426647 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426647&action=review

> Source/WebCore/Modules/highlight/AppHighlightStorage.h:66
> +    bool attemptToRestoreHighlightAndScroll(AppHighlightRangeData&, ScrollToHighlight);

still missing this newline
Comment 19 Megan Gardner 2021-04-20 23:14:03 PDT
Created attachment 426649 [details]
Patch for landing
Comment 20 EWS 2021-04-21 00:18:24 PDT
Committed r276347 (236825@main): <https://commits.webkit.org/236825@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426649 [details].
Comment 21 Megan Gardner 2021-04-21 10:56:22 PDT
rdar://76166593