Bug 255953 – Play Animation and Pause Animation action sheet items don't appear when a sibling link covers the animation

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Tyler Wilcock]

Testcase:

<a href="#foo">
    <img src="animated-red-green-blue-repeat-infinite.gif" width="223" height="223" alt="Alt text">
</a>
<div style="width: 233px; height: 223px; position: absolute; top: 8px;">
    <a href="#bar" style="width: 223px; height: 223px">
        <div style="width: 233px; height: 223px;"></div>
    </a>
</div>

[Radar WebKit Bug Importer]

<rdar://problem/108524363>

[Tyler Wilcock]

Created attachment 466082 [details]
Patch

[Tyler Wilcock]

Created attachment 466083 [details]
Patch

[Tyler Wilcock]

Created attachment 466120 [details]
Patch

[Wenson Hsieh]

Comment on attachment 466120 [details]
Patch

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

> Source/WebKit/Shared/ios/InteractionInformationRequest.cpp:98
> +    if (other.gatherAnimations != gatherAnimations)

Nit - shouldn't this be `other.gatherAnimations && !gatherAnimations`? In other words, the currently cached position information is only invalid for the incoming request in the case where we haven't gathered animations, but the incoming request wants to gather animations.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:519
> +- (BOOL)_allowAnimationControls;

Nit - `WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA))` here (we still specify availability annotations for SPI, since there may be internal clients that may depend on this information).

> Source/WebKit/UIProcess/API/Cocoa/_WKActivatedElementInfo.mm:138
> +- (instancetype)_initWithType:(_WKActivatedElementType)type URL:(NSURL *)url imageURL:(NSURL *)imageURL location:(const WebCore::IntPoint&)location title:(NSString *)title ID:(NSString *)ID rect:(CGRect)rect image:(WebKit::ShareableBitmap*)image imageMIMEType:(NSString *)imageMIMEType isAnimatedImage:(BOOL)isAnimatedImage isAnimating:(BOOL)isAnimating canShowAnimationControls:(BOOL)canShowAnimationControls animationsUnderElement:(Vector<WebCore::ElementAnimationContext>)animationsUnderElement userInfo:(NSDictionary *)userInfo

Nit - either `Vector<WebCore::ElementAnimationContext>&&` or `const Vector<WebCore::ElementAnimationContext>&` to avoid unnecessary copies.

> Source/WebKit/UIProcess/API/Cocoa/_WKActivatedElementInfo.mm:214
> +- (Vector<WebCore::ElementAnimationContext>)_animationsUnderElement

Nit - same here, `const Vector<WebCore::ElementAnimationContext>&` so that we don't copy unless necessary.

> Source/WebKit/UIProcess/WebPageProxy.h:927
> +    void performActionOnElements(uint32_t action, Vector<WebCore::ElementContext>);

Nit - `Vector<WebCore::ElementContext>&&`.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:586
> +void WebPageProxy::performActionOnElements(uint32_t action, Vector<WebCore::ElementContext> elements)

Nit - either `Vector<WebCore::ElementContext>&&` here with `WTFMove(elements)`, or `const Vector<WebCore::ElementContext>&`.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3302
> +        if (std::optional elementContext = page.contextForElement(*element))

Nit - s/std::optional/auto/

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3303
> +            info.animationsAtPoint.append({ *elementContext, rendererAndImage->second.isAnimating() });

You can `WTFMove(*elementContext)` here.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3433
> +        if (auto* imageElement = dynamicDowncast<HTMLImageElement>(element))

Nit - not new in this patch, but I would use `RefPtr` here since `setAllowsAnimation` isn't a trivial setter.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3436
> +        if (auto* imageElement = dynamicDowncast<HTMLImageElement>(element))

Ditto.

[Tyler Wilcock]

Created attachment 466166 [details]
Patch

[Tyler Wilcock]

(In reply to Wenson Hsieh from comment #5)
> Comment on attachment 466120 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=466120&action=review
> 
> > Source/WebKit/Shared/ios/InteractionInformationRequest.cpp:98
> > +    if (other.gatherAnimations != gatherAnimations)
> 
> Nit - shouldn't this be `other.gatherAnimations && !gatherAnimations`? In
> other words, the currently cached position information is only invalid for
> the incoming request in the case where we haven't gathered animations, but
> the incoming request wants to gather animations.
Yes, great catch, thanks! Addressed all comments in latest patch.

[EWS]

Committed 263598@main (31091c93475d): <https://commits.webkit.org/263598@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 466166 [details].