Bug 225727 - Add textIndicator bounce for AppHighlights on scroll.
Summary: Add textIndicator bounce for AppHighlights on scroll.
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-05-12 18:32 PDT by Megan Gardner
Modified: 2021-05-13 14:58 PDT (History)
10 users (show)

See Also:


Attachments
Patch (18.96 KB, patch)
2021-05-12 18:46 PDT, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (18.92 KB, patch)
2021-05-12 19:05 PDT, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (19.16 KB, patch)
2021-05-12 19:29 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (51.08 KB, patch)
2021-05-13 02:39 PDT, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (50.89 KB, patch)
2021-05-13 02:44 PDT, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (50.94 KB, patch)
2021-05-13 02:47 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (50.74 KB, patch)
2021-05-13 03:00 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (50.66 KB, patch)
2021-05-13 03:02 PDT, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (50.59 KB, patch)
2021-05-13 03:11 PDT, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (51.00 KB, patch)
2021-05-13 03:22 PDT, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (51.17 KB, patch)
2021-05-13 03:31 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
a modest proposal (51.44 KB, patch)
2021-05-13 05:32 PDT, Tim Horton
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
a modest proposal (51.44 KB, patch)
2021-05-13 05:39 PDT, Tim Horton
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (51.00 KB, patch)
2021-05-13 10:27 PDT, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (50.99 KB, patch)
2021-05-13 10:50 PDT, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (51.48 KB, patch)
2021-05-13 12:51 PDT, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (51.52 KB, patch)
2021-05-13 13:13 PDT, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
a modest proposal (52.65 KB, patch)
2021-05-13 13:36 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (55.12 KB, patch)
2021-05-13 13:38 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (55.10 KB, patch)
2021-05-13 14:43 PDT, Megan Gardner
megan_gardner: commit-queue-
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-05-12 18:32:32 PDT
Add textIndicator to bounce for AppHighlights on scroll.
Comment 1 Megan Gardner 2021-05-12 18:46:58 PDT
Created attachment 428446 [details]
Patch
Comment 2 Megan Gardner 2021-05-12 18:47:40 PDT
rdar://76158572
Comment 3 Megan Gardner 2021-05-12 19:05:24 PDT
Created attachment 428447 [details]
Patch
Comment 4 Tim Horton 2021-05-12 19:10:56 PDT
Comment on attachment 428446 [details]
Patch

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

> Source/WebCore/page/cocoa/WebTextIndicatorLayer.h:38
> +constexpr CGFloat dropShadowBlurRadius = 2;
> +constexpr CGFloat rimShadowBlurRadius = 1;
> +constexpr CFTimeInterval bounceAnimationDuration = 0.12;
> +constexpr CFTimeInterval timeBeforeFadeStarts = bounceAnimationDuration + 0.2;
> +constexpr CGFloat midBounceScale = 1.25;

These should either be static methods on WebTextIndicatorLayer or in the WebCore C++ namespace, but not floating here at the toplevel.

> Source/WebCore/page/cocoa/WebTextIndicatorLayer.mm:131
> +    self.anchorPoint = CGPointZero;

Please double-check macOS with this change.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:687
> +

weird newline balance problem

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:9601
> +    CGRect frame = _textIndicator.get()->textBoundingRectInRootViewCoordinates();

definitely don't need this .get()

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:9610
> +    [self performSelector:@selector(startFadeOut) withObject:self afterDelay:timeBeforeFadeStarts];

Later (not in this patch) we should factor this out into the layer (or some other shared place).

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:9615
> +

Weird newline

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:9621
> +    if (textIndicator && [_textIndicatorLayer indicatorWantsManualAnimation:*textIndicator] && [_textIndicatorLayer hasCompletedAnimation] && animation == WebCore::TextIndicatorWindowDismissalAnimation::FadeOut) {

This is technically dead code on iOS, but I guess it's also fine to handle it?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:9639
> +    [_textIndicatorLayer removeFromSuperlayer];

Surely this should also nil out _textIndicatorLayer
Comment 5 Wenson Hsieh 2021-05-12 19:27:13 PDT
Comment on attachment 428447 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:9647
> +    [_textIndicatorLayer hideWithCompletionHandler:[self] {
> +        [self teardownTextIndicatorLayer];
> +    }];

This should probably weakly capture self
Comment 6 Megan Gardner 2021-05-12 19:29:45 PDT
Created attachment 428449 [details]
Patch
Comment 7 Megan Gardner 2021-05-13 02:39:16 PDT
Created attachment 428472 [details]
Patch
Comment 8 Megan Gardner 2021-05-13 02:44:16 PDT
Created attachment 428473 [details]
Patch
Comment 9 Megan Gardner 2021-05-13 02:47:10 PDT
Created attachment 428474 [details]
Patch
Comment 10 Tim Horton 2021-05-13 02:49:38 PDT
Comment on attachment 428474 [details]
Patch

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

> Source/WebKit/UIProcess/ios/PageClientImplIOS.mm:484
> +#if ENABLE(APP_HIGHLIGHTS)

What! Why is this guarded?

> Source/WebKit/UIProcess/ios/PageClientImplIOS.mm:493
> +#if ENABLE(APP_HIGHLIGHTS)

No

> Source/WebKit/UIProcess/ios/PageClientImplIOS.mm:502
> +#if ENABLE(APP_HIGHLIGHTS)

No

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:136
> +#if ENABLE(APP_HIGHLIGHTS)

Stop all of this madness :)

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:9590
> +#if ENABLE(APP_HIGHLIGHTS)

All of it!
Comment 11 Tim Horton 2021-05-13 02:57:45 PDT
Comment on attachment 428474 [details]
Patch

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

> Source/WebCore/page/TextIndicator.h:42
> +#if PLATFORM(MAC) || PLATFORM(IOS)

PLATFORM(COCOA) and a fixme to adopt WebCore types here
Comment 12 Megan Gardner 2021-05-13 03:00:56 PDT
Created attachment 428479 [details]
Patch
Comment 13 Megan Gardner 2021-05-13 03:02:55 PDT
Created attachment 428480 [details]
Patch
Comment 14 Megan Gardner 2021-05-13 03:11:22 PDT
Created attachment 428482 [details]
Patch
Comment 15 Megan Gardner 2021-05-13 03:22:42 PDT
Created attachment 428483 [details]
Patch
Comment 16 Megan Gardner 2021-05-13 03:31:14 PDT
Created attachment 428484 [details]
Patch
Comment 17 Tim Horton 2021-05-13 05:32:09 PDT
Created attachment 428492 [details]
a modest proposal

without all the ifdefs, for EWS
Comment 18 Tim Horton 2021-05-13 05:33:02 PDT
Sorry, forgot --no-obsolete. I put yours back. But please consider the ifdefless version, if it builds!
Comment 19 Tim Horton 2021-05-13 05:39:07 PDT
Created attachment 428497 [details]
a modest proposal

without all the ifdefs, for EWS
Comment 20 Megan Gardner 2021-05-13 10:27:04 PDT
Created attachment 428527 [details]
Patch
Comment 21 Megan Gardner 2021-05-13 10:50:15 PDT
Created attachment 428531 [details]
Patch
Comment 22 Tim Horton 2021-05-13 11:11:23 PDT
Comment on attachment 428531 [details]
Patch

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

> Source/WebCore/loader/EmptyClients.cpp:556
> +void EmptyChromeClient::setTextIndicator(const TextIndicatorData&) const

My version of the patch fixes all these things but: 

Shouldn't be in the ifdef

> Source/WebCore/loader/EmptyClients.h:150
> +    void setTextIndicator(const TextIndicatorData&) const final;

Shouldn't be in the ifdef

> Source/WebCore/page/ChromeClient.h:317
> +    virtual void setTextIndicator(const TextIndicatorData&) const = 0;

Shouldn't be in the ifdef

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1273
> +}

This is misplaced

> Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.h:156
> +    void setTextIndicator(const WebCore::TextIndicatorData&) const final;

Shouldn't be in the ifdef
Comment 23 Tim Horton 2021-05-13 11:15:07 PDT
(And needs a empty implementation in the windows chrome client)
Comment 24 Megan Gardner 2021-05-13 12:51:50 PDT
Created attachment 428549 [details]
Patch
Comment 25 Megan Gardner 2021-05-13 13:13:30 PDT
Created attachment 428553 [details]
Patch
Comment 26 Tim Horton 2021-05-13 13:36:15 PDT
Created attachment 428556 [details]
a modest proposal

without all the ifdefs, for EWS
Comment 27 Megan Gardner 2021-05-13 13:38:54 PDT
Created attachment 428558 [details]
Patch
Comment 28 Megan Gardner 2021-05-13 14:43:10 PDT
Created attachment 428563 [details]
Patch for landing
Comment 29 Megan Gardner 2021-05-13 14:58:47 PDT
https://trac.webkit.org/changeset/277453/webkit