Add textIndicator to bounce for AppHighlights on scroll.
Created attachment 428446 [details] Patch
rdar://76158572
Created attachment 428447 [details] Patch
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 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
Created attachment 428449 [details] Patch
Created attachment 428472 [details] Patch
Created attachment 428473 [details] Patch
Created attachment 428474 [details] Patch
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 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
Created attachment 428479 [details] Patch
Created attachment 428480 [details] Patch
Created attachment 428482 [details] Patch
Created attachment 428483 [details] Patch
Created attachment 428484 [details] Patch
Created attachment 428492 [details] a modest proposal without all the ifdefs, for EWS
Sorry, forgot --no-obsolete. I put yours back. But please consider the ifdefless version, if it builds!
Created attachment 428497 [details] a modest proposal without all the ifdefs, for EWS
Created attachment 428527 [details] Patch
Created attachment 428531 [details] Patch
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
(And needs a empty implementation in the windows chrome client)
Created attachment 428549 [details] Patch
Created attachment 428553 [details] Patch
Created attachment 428556 [details] a modest proposal without all the ifdefs, for EWS
Created attachment 428558 [details] Patch
Created attachment 428563 [details] Patch for landing
https://trac.webkit.org/changeset/277453/webkit