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>
<rdar://problem/108524363>
Created attachment 466082 [details] Patch
Created attachment 466083 [details] Patch
Created attachment 466120 [details] Patch
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.
Created attachment 466166 [details] Patch
(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.
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].