- Revision
- 278189
- Author
- drou...@apple.com
- Date
- 2021-05-27 22:02:50 -0700 (Thu, 27 May 2021)
Log Message
Sampled Page Top Color: allow snapshotting of elements with CSS animations/transitions if they're paused
https://bugs.webkit.org/show_bug.cgi?id=226313
<rdar://problem/78534076>
Reviewed by Tim Horton.
Source/WebCore:
The primary concern of sampling elements with CSS animations/transitions is that it
introduces some indeterminism in when the sampling happens vs how far progressed the CSS
animation/transition is. As an example, sampling from a page with an inline `<style>` that
applies a CSS animation to fade in the CSS `background-color` from `white` to `black` is
more likely to get a color closer to `black` than if that same CSS was in an external
uncached resource. This wouldn't make for a great experience, so r277044 made it so that any
CSS animations/transitions caused the sampling logic to bail, regardless of the state of the
CSS animation/transition. This is only really an issue for CSS animations/transitions that
are actively running, however, not ones that have yet to run or have already finished. It's
still possible that two loads of the same page could result in different colors (or bails)
depending on how quickly the CSS animation/transition runs or (if the CSS is in an external
resource) how long the containing resource takes to load, but with this patch it's now a
binary state (sample or bail) instead of an indeterminate range of possible sampled colors.
Tests: SampledPageTopColor.HitTestBeforeCSSTransition
SampledPageTopColor.HitTestDuringCSSTransition
SampledPageTopColor.HitTestAfterCSSTransition
SampledPageTopColor.HitTestBeforeCSSAnimation
SampledPageTopColor.HitTestDuringCSSAnimation
SampledPageTopColor.HitTestAfterCSSAnimation
* page/PageColorSampler.cpp:
(WebCore::isValidSampleLocation):
Use `Styleable` instead of `RenderStyle` to get more information about active CSS animations/transitions.
Tools:
* TestWebKitAPI/Tests/WebKitCocoa/SampledPageTopColor.mm:
(waitForSampledPageTopColorToChange): Added.
(waitForSampledPageTopColorToChangeForHTML):
(TEST.SampledPageTopColor.HitTestBeforeCSSTransition): Added.
(TEST.SampledPageTopColor.HitTestDuringCSSTransition): Added.
(TEST.SampledPageTopColor.HitTestAfterCSSTransition): Added.
(TEST.SampledPageTopColor.HitTestBeforeCSSAnimation): Added.
(TEST.SampledPageTopColor.HitTestDuringCSSAnimation): Added.
(TEST.SampledPageTopColor.HitTestAfterCSSAnimation): Added.
(TEST.SampledPageTopColor.HitTestCSSAnimation): Deleted.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (278188 => 278189)
--- trunk/Source/WebCore/ChangeLog 2021-05-28 04:16:18 UTC (rev 278188)
+++ trunk/Source/WebCore/ChangeLog 2021-05-28 05:02:50 UTC (rev 278189)
@@ -1,3 +1,36 @@
+2021-05-27 Devin Rousso <drou...@apple.com>
+
+ Sampled Page Top Color: allow snapshotting of elements with CSS animations/transitions if they're paused
+ https://bugs.webkit.org/show_bug.cgi?id=226313
+ <rdar://problem/78534076>
+
+ Reviewed by Tim Horton.
+
+ The primary concern of sampling elements with CSS animations/transitions is that it
+ introduces some indeterminism in when the sampling happens vs how far progressed the CSS
+ animation/transition is. As an example, sampling from a page with an inline `<style>` that
+ applies a CSS animation to fade in the CSS `background-color` from `white` to `black` is
+ more likely to get a color closer to `black` than if that same CSS was in an external
+ uncached resource. This wouldn't make for a great experience, so r277044 made it so that any
+ CSS animations/transitions caused the sampling logic to bail, regardless of the state of the
+ CSS animation/transition. This is only really an issue for CSS animations/transitions that
+ are actively running, however, not ones that have yet to run or have already finished. It's
+ still possible that two loads of the same page could result in different colors (or bails)
+ depending on how quickly the CSS animation/transition runs or (if the CSS is in an external
+ resource) how long the containing resource takes to load, but with this patch it's now a
+ binary state (sample or bail) instead of an indeterminate range of possible sampled colors.
+
+ Tests: SampledPageTopColor.HitTestBeforeCSSTransition
+ SampledPageTopColor.HitTestDuringCSSTransition
+ SampledPageTopColor.HitTestAfterCSSTransition
+ SampledPageTopColor.HitTestBeforeCSSAnimation
+ SampledPageTopColor.HitTestDuringCSSAnimation
+ SampledPageTopColor.HitTestAfterCSSAnimation
+
+ * page/PageColorSampler.cpp:
+ (WebCore::isValidSampleLocation):
+ Use `Styleable` instead of `RenderStyle` to get more information about active CSS animations/transitions.
+
2021-05-27 Said Abou-Hallawa <s...@apple.com>
Values of keySplines control points must all be in the range 0 to 1
Modified: trunk/Source/WebCore/page/PageColorSampler.cpp (278188 => 278189)
--- trunk/Source/WebCore/page/PageColorSampler.cpp 2021-05-28 04:16:18 UTC (rev 278188)
+++ trunk/Source/WebCore/page/PageColorSampler.cpp 2021-05-28 05:02:50 UTC (rev 278189)
@@ -28,6 +28,7 @@
#include "ContentfulPaintChecker.h"
#include "Document.h"
+#include "Element.h"
#include "Frame.h"
#include "FrameSnapshotting.h"
#include "FrameView.h"
@@ -39,6 +40,7 @@
#include "IntPoint.h"
#include "IntRect.h"
#include "IntSize.h"
+#include "Logging.h"
#include "Node.h"
#include "Page.h"
#include "PixelBuffer.h"
@@ -47,6 +49,8 @@
#include "RenderObject.h"
#include "RenderStyle.h"
#include "Settings.h"
+#include "Styleable.h"
+#include "WebAnimation.h"
#include <wtf/ListHashSet.h>
#include <wtf/OptionSet.h>
#include <wtf/Optional.h>
@@ -75,17 +79,31 @@
if (is<RenderImage>(renderer) || renderer->style().hasBackgroundImage())
return false;
+ if (!is<Element>(node))
+ continue;
+
+ auto& element = downcast<Element>(node);
+ auto styleable = Styleable::fromElement(element);
+
// Skip nodes with animations as the sample may get an odd color if the animation is in-progress.
- if (renderer->style().hasTransitions() || renderer->style().hasAnimations())
+ if (styleable.hasRunningTransitions())
return false;
+ if (auto* animations = styleable.animations()) {
+ for (auto& animation : *animations) {
+ if (!animation)
+ continue;
+ if (animation->playState() == WebAnimation::PlayState::Running)
+ return false;
+ }
+ }
// Skip `<canvas>` but only if they've been drawn into. Guess this by seeing if there's already
// a `CanvasRenderingContext`, which is only created by _javascript_.
- if (is<HTMLCanvasElement>(node) && downcast<HTMLCanvasElement>(node).renderingContext())
+ if (is<HTMLCanvasElement>(element) && downcast<HTMLCanvasElement>(element).renderingContext())
return false;
// Skip 3rd-party `<iframe>` as the content likely won't match the rest of the page.
- if (is<HTMLIFrameElement>(node) && !areRegistrableDomainsEqual(downcast<HTMLIFrameElement>(node).location(), document.url()))
+ if (is<HTMLIFrameElement>(element) && !areRegistrableDomainsEqual(downcast<HTMLIFrameElement>(element).location(), document.url()))
return false;
}
Modified: trunk/Tools/ChangeLog (278188 => 278189)
--- trunk/Tools/ChangeLog 2021-05-28 04:16:18 UTC (rev 278188)
+++ trunk/Tools/ChangeLog 2021-05-28 05:02:50 UTC (rev 278189)
@@ -1,3 +1,22 @@
+2021-05-27 Devin Rousso <drou...@apple.com>
+
+ Sampled Page Top Color: allow snapshotting of elements with CSS animations/transitions if they're paused
+ https://bugs.webkit.org/show_bug.cgi?id=226313
+ <rdar://problem/78534076>
+
+ Reviewed by Tim Horton.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/SampledPageTopColor.mm:
+ (waitForSampledPageTopColorToChange): Added.
+ (waitForSampledPageTopColorToChangeForHTML):
+ (TEST.SampledPageTopColor.HitTestBeforeCSSTransition): Added.
+ (TEST.SampledPageTopColor.HitTestDuringCSSTransition): Added.
+ (TEST.SampledPageTopColor.HitTestAfterCSSTransition): Added.
+ (TEST.SampledPageTopColor.HitTestBeforeCSSAnimation): Added.
+ (TEST.SampledPageTopColor.HitTestDuringCSSAnimation): Added.
+ (TEST.SampledPageTopColor.HitTestAfterCSSAnimation): Added.
+ (TEST.SampledPageTopColor.HitTestCSSAnimation): Deleted.
+
2021-05-27 Jonathan Bedard <jbed...@apple.com>
[webkitcorey] Gracefully handle CNTRL-C in TaskPool
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/SampledPageTopColor.mm (278188 => 278189)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/SampledPageTopColor.mm 2021-05-28 04:16:18 UTC (rev 278188)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/SampledPageTopColor.mm 2021-05-28 05:02:50 UTC (rev 278189)
@@ -90,16 +90,26 @@
return adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 800, 600) configuration:configuration.get()]);
}
-static void waitForSampledPageTopColorToChangeForHTML(TestWKWebView *webView, String&& html)
+static void waitForSampledPageTopColorToChange(TestWKWebView *webView, Function<void()>&& trigger = nullptr)
{
bool done = false;
auto sampledPageTopColorObserver = adoptNS([[TestKVOWrapper alloc] initWithObservable:webView keyPath:@"_sampledPageTopColor" callback:[&] {
done = true;
}]);
- [webView synchronouslyLoadHTMLStringAndWaitUntilAllImmediateChildFramesPaint:WTFMove(html)];
+
+ if (trigger)
+ trigger();
+
TestWebKitAPI::Util::run(&done);
}
+static void waitForSampledPageTopColorToChangeForHTML(TestWKWebView *webView, String&& html)
+{
+ waitForSampledPageTopColorToChange(webView, [webView, html = WTFMove(html)] () mutable {
+ [webView synchronouslyLoadHTMLStringAndWaitUntilAllImmediateChildFramesPaint:WTFMove(html)];
+ });
+}
+
static String createHTMLGradientWithColorStops(String&& direction, Vector<String>&& colorStops)
{
EXPECT_GE(colorStops.size(), 2UL);
@@ -312,15 +322,86 @@
EXPECT_NULL([webView _sampledPageTopColor]);
}
-TEST(SampledPageTopColor, HitTestCSSAnimation)
+TEST(SampledPageTopColor, HitTestBeforeCSSTransition)
{
auto webView = createWebViewWithSampledPageTopColorMaxDifference(5);
EXPECT_NULL([webView _sampledPageTopColor]);
- [webView synchronouslyLoadHTMLStringAndWaitUntilAllImmediateChildFramesPaint:@"<style>@keyframes fadeIn { from { opacity: 0; } }</style><body style='animation: fadeIn 1s infinite alternate'>Test"];
+ waitForSampledPageTopColorToChangeForHTML(webView.get(), @"<body style='margin: 0; transition: background-color 1s'>Test");
+ EXPECT_EQ(WebCore::Color([webView _sampledPageTopColor].CGColor), WebCore::Color::white);
+}
+
+TEST(SampledPageTopColor, HitTestDuringCSSTransition)
+{
+ auto webView = createWebViewWithSampledPageTopColorMaxDifference(5);
EXPECT_NULL([webView _sampledPageTopColor]);
+
+ [webView synchronouslyLoadHTMLStringAndWaitUntilAllImmediateChildFramesPaint:@"<body style='margin: 0; transition: background-color 1000s'>"];
+ [webView objectByEvaluatingJavaScript:@"document.body.style.setProperty('background-color', 'red')"];
+
+ TestWebKitAPI::Util::sleep(1);
+
+ // Not setting this until now prevents the sampling logic from running because without it the page isn't considered contentful.
+ [webView objectByEvaluatingJavaScript:@"document.body.textContent = 'Test'"];
+ [webView waitForNextPresentationUpdate];
+ EXPECT_NULL([webView _sampledPageTopColor]);
}
+TEST(SampledPageTopColor, HitTestAfterCSSTransition)
+{
+ auto webView = createWebViewWithSampledPageTopColorMaxDifference(5);
+ EXPECT_NULL([webView _sampledPageTopColor]);
+
+ [webView synchronouslyLoadHTMLStringAndWaitUntilAllImmediateChildFramesPaint:@"<body style='margin: 0; transition: background-color 0.1s'>"];
+ [webView objectByEvaluatingJavaScript:@"document.body.style.setProperty('background-color', 'red')"];
+
+ TestWebKitAPI::Util::sleep(1);
+
+ // Not setting this until now prevents the sampling logic from running because without it the page isn't considered contentful.
+ [webView objectByEvaluatingJavaScript:@"document.body.textContent = 'Test'"];
+ waitForSampledPageTopColorToChange(webView.get());
+ EXPECT_EQ(WebCore::Color([webView _sampledPageTopColor].CGColor), WebCore::Color::red);
+}
+
+TEST(SampledPageTopColor, HitTestBeforeCSSAnimation)
+{
+ auto webView = createWebViewWithSampledPageTopColorMaxDifference(5);
+ EXPECT_NULL([webView _sampledPageTopColor]);
+
+ waitForSampledPageTopColorToChangeForHTML(webView.get(), @"<style>@keyframes changeBackgroundRed { to { background-color: red; } }</style><body style='margin: 0; animation: changeBackgroundRed 1s forwards paused'>Test");
+ EXPECT_EQ(WebCore::Color([webView _sampledPageTopColor].CGColor), WebCore::Color::white);
+}
+
+TEST(SampledPageTopColor, HitTestDuringCSSAnimation)
+{
+ auto webView = createWebViewWithSampledPageTopColorMaxDifference(5);
+ EXPECT_NULL([webView _sampledPageTopColor]);
+
+ [webView synchronouslyLoadHTMLStringAndWaitUntilAllImmediateChildFramesPaint:@"<style>@keyframes changeBackgroundRed { to { background-color: red; } }</style><body style='margin: 0; animation: changeBackgroundRed 1000s forwards'>"];
+
+ TestWebKitAPI::Util::sleep(1);
+
+ // Not setting this until now prevents the sampling logic from running because without it the page isn't considered contentful.
+ [webView objectByEvaluatingJavaScript:@"document.body.textContent = 'Test'"];
+ [webView waitForNextPresentationUpdate];
+ EXPECT_NULL([webView _sampledPageTopColor]);
+}
+
+TEST(SampledPageTopColor, HitTestAfterCSSAnimation)
+{
+ auto webView = createWebViewWithSampledPageTopColorMaxDifference(5);
+ EXPECT_NULL([webView _sampledPageTopColor]);
+
+ [webView synchronouslyLoadHTMLStringAndWaitUntilAllImmediateChildFramesPaint:@"<style>@keyframes changeBackgroundRed { to { background-color: red; } }</style><body style='margin: 0; animation: changeBackgroundRed 0.1s forwards'>"];
+
+ TestWebKitAPI::Util::sleep(1);
+
+ // Not setting this until now prevents the sampling logic from running because without it the page isn't considered contentful.
+ [webView objectByEvaluatingJavaScript:@"document.body.textContent = 'Test'"];
+ waitForSampledPageTopColorToChange(webView.get());
+ EXPECT_EQ(WebCore::Color([webView _sampledPageTopColor].CGColor), WebCore::Color::red);
+}
+
TEST(SampledPageTopColor, HitTestCSSPointerEventsNone)
{
auto webView = createWebViewWithSampledPageTopColorMaxDifference(5);
