Bug 225727 – Add textIndicator bounce for AppHighlights on scroll.

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Add textIndicator to bounce for AppHighlights on scroll.

[Megan Gardner]

Created attachment 428446 [details]
Patch

[Megan Gardner]

rdar://76158572

[Megan Gardner]

Created attachment 428447 [details]
Patch

[Tim Horton]

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

[Wenson Hsieh]

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

[Megan Gardner]

Created attachment 428449 [details]
Patch

[Megan Gardner]

Created attachment 428472 [details]
Patch

[Megan Gardner]

Created attachment 428473 [details]
Patch

[Megan Gardner]

Created attachment 428474 [details]
Patch

[Tim Horton]

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!

[Tim Horton]

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

[Megan Gardner]

Created attachment 428479 [details]
Patch

[Megan Gardner]

Created attachment 428480 [details]
Patch

[Megan Gardner]

Created attachment 428482 [details]
Patch

[Megan Gardner]

Created attachment 428483 [details]
Patch

[Megan Gardner]

Created attachment 428484 [details]
Patch

[Tim Horton]

Created attachment 428492 [details]
a modest proposal

without all the ifdefs, for EWS

[Tim Horton]

Sorry, forgot --no-obsolete. I put yours back. But please consider the ifdefless version, if it builds!

[Tim Horton]

Created attachment 428497 [details]
a modest proposal

without all the ifdefs, for EWS

[Megan Gardner]

Created attachment 428527 [details]
Patch

[Megan Gardner]

Created attachment 428531 [details]
Patch

[Tim Horton]

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

[Tim Horton]

(And needs a empty implementation in the windows chrome client)

[Megan Gardner]

Created attachment 428549 [details]
Patch

[Megan Gardner]

Created attachment 428553 [details]
Patch

[Tim Horton]

Created attachment 428556 [details]
a modest proposal

without all the ifdefs, for EWS

[Megan Gardner]

Created attachment 428558 [details]
Patch

[Megan Gardner]

Created attachment 428563 [details]
Patch for landing

[Megan Gardner]

https://trac.webkit.org/changeset/277453/webkit