Make sure 'font-display: optional' doesn't cause relayout
This patch ensures that an 'optional' web font never causes relayout:
- When document rendering starts, any 'optional' web font that hasn't
been loaded enters the failure period directly.
- When an @font-face rule is added, if document rendering has started
and the font is not available from memory cache, then it enters the
failure period directly.
In this way, any element attempting to use such 'optional' fonts will
always use a font that's further down in the 'font-family' list during
the document's lifetime.
Bug: 1040632
Change-Id: Idde675a8dddadf15d22c13b95db649410fd9cd0e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2057502
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749080}
diff --git a/third_party/blink/renderer/core/css/remote_font_face_source.cc b/third_party/blink/renderer/core/css/remote_font_face_source.cc
index 6a1ea49..c5c928be 100644
--- a/third_party/blink/renderer/core/css/remote_font_face_source.cc
+++ b/third_party/blink/renderer/core/css/remote_font_face_source.cc
@@ -5,6 +5,7 @@
#include "third_party/blink/renderer/core/css/remote_font_face_source.h"
#include "base/metrics/histogram_functions.h"
+#include "third_party/blink/public/common/features.h"
#include "third_party/blink/public/mojom/feature_policy/feature_policy_feature.mojom-blink.h"
#include "third_party/blink/public/platform/task_type.h"
#include "third_party/blink/public/platform/web_effective_connection_type.h"
@@ -27,58 +28,71 @@
namespace blink {
-namespace {
-
-RemoteFontFaceSource::DisplayPeriod ComputePeriod(
- FontDisplay displayValue,
- RemoteFontFaceSource::Phase phase,
- bool is_intervention_triggered) {
- switch (displayValue) {
+RemoteFontFaceSource::DisplayPeriod RemoteFontFaceSource::ComputePeriod()
+ const {
+ switch (display_) {
case kFontDisplayAuto:
- if (is_intervention_triggered)
- return RemoteFontFaceSource::kSwapPeriod;
+ if (is_intervention_triggered_)
+ return kSwapPeriod;
FALLTHROUGH;
case kFontDisplayBlock:
- switch (phase) {
- case RemoteFontFaceSource::kNoLimitExceeded:
- case RemoteFontFaceSource::kShortLimitExceeded:
- return RemoteFontFaceSource::kBlockPeriod;
- case RemoteFontFaceSource::kLongLimitExceeded:
- return RemoteFontFaceSource::kSwapPeriod;
+ switch (phase_) {
+ case kNoLimitExceeded:
+ case kShortLimitExceeded:
+ return kBlockPeriod;
+ case kLongLimitExceeded:
+ return kSwapPeriod;
}
case kFontDisplaySwap:
- return RemoteFontFaceSource::kSwapPeriod;
+ return kSwapPeriod;
case kFontDisplayFallback:
- switch (phase) {
- case RemoteFontFaceSource::kNoLimitExceeded:
- return RemoteFontFaceSource::kBlockPeriod;
- case RemoteFontFaceSource::kShortLimitExceeded:
- return RemoteFontFaceSource::kSwapPeriod;
- case RemoteFontFaceSource::kLongLimitExceeded:
- return RemoteFontFaceSource::kFailurePeriod;
+ switch (phase_) {
+ case kNoLimitExceeded:
+ return kBlockPeriod;
+ case kShortLimitExceeded:
+ return kSwapPeriod;
+ case kLongLimitExceeded:
+ return kFailurePeriod;
}
- case kFontDisplayOptional:
- switch (phase) {
- case RemoteFontFaceSource::kNoLimitExceeded:
- return RemoteFontFaceSource::kBlockPeriod;
- case RemoteFontFaceSource::kShortLimitExceeded:
- case RemoteFontFaceSource::kLongLimitExceeded:
- return RemoteFontFaceSource::kFailurePeriod;
+ case kFontDisplayOptional: {
+ const bool use_phase_value =
+ !base::FeatureList::IsEnabled(
+ features::kFontPreloadingDelaysRendering) ||
+ !GetDocument();
+
+ if (use_phase_value) {
+ switch (phase_) {
+ case kNoLimitExceeded:
+ return kBlockPeriod;
+ case kShortLimitExceeded:
+ case kLongLimitExceeded:
+ return kFailurePeriod;
+ }
}
+ // We simply skip the block period, as we should never render invisible
+ // fallback for 'font-display: optional'.
+
+ if (GetDocument()->GetFontPreloadManager().RenderingHasBegun()) {
+ if (FinishedFromMemoryCache() ||
+ finished_before_document_rendering_begin_)
+ return kSwapPeriod;
+ return kFailurePeriod;
+ }
+
+ return kSwapPeriod;
+ }
case kFontDisplayEnumMax:
NOTREACHED();
break;
}
NOTREACHED();
- return RemoteFontFaceSource::kSwapPeriod;
+ return kSwapPeriod;
}
-} // namespace
-
RemoteFontFaceSource::RemoteFontFaceSource(CSSFontFace* css_font_face,
FontSelector* font_selector,
FontDisplay display)
@@ -90,13 +104,18 @@
font_selector,
ReportOptions::kDoNotReport)),
phase_(kNoLimitExceeded),
- is_intervention_triggered_(ShouldTriggerWebFontsIntervention()) {
+ is_intervention_triggered_(ShouldTriggerWebFontsIntervention()),
+ finished_before_document_rendering_begin_(false) {
DCHECK(face_);
- period_ = ComputePeriod(display_, phase_, is_intervention_triggered_);
+ period_ = ComputePeriod();
}
RemoteFontFaceSource::~RemoteFontFaceSource() = default;
+Document* RemoteFontFaceSource::GetDocument() const {
+ return Document::DynamicFrom(font_selector_->GetExecutionContext());
+}
+
void RemoteFontFaceSource::Dispose() {
ClearResource();
PruneTable();
@@ -147,6 +166,17 @@
ClearResource();
PruneTable();
+
+ if (GetDocument() &&
+ !GetDocument()->GetFontPreloadManager().RenderingHasBegun()) {
+ finished_before_document_rendering_begin_ = true;
+ }
+
+ if (FinishedFromMemoryCache())
+ period_ = kNotApplicablePeriod;
+ else
+ UpdatePeriod();
+
if (face_->FontLoaded(this)) {
font_selector_->FontFaceInvalidated();
@@ -187,8 +217,7 @@
}
void RemoteFontFaceSource::UpdatePeriod() {
- DisplayPeriod new_period =
- ComputePeriod(display_, phase_, is_intervention_triggered_);
+ DisplayPeriod new_period = ComputePeriod();
// Fallback font is invisible iff the font is loading and in the block period.
// Invalidate the font if its fallback visibility has changed.
diff --git a/third_party/blink/renderer/core/css/remote_font_face_source.h b/third_party/blink/renderer/core/css/remote_font_face_source.h
index d53ecb4..5476391d 100644
--- a/third_party/blink/renderer/core/css/remote_font_face_source.h
+++ b/third_party/blink/renderer/core/css/remote_font_face_source.h
@@ -13,6 +13,7 @@
namespace blink {
class CSSFontFace;
+class Document;
class FontSelector;
class FontCustomPlatformData;
@@ -23,9 +24,6 @@
public:
enum Phase { kNoLimitExceeded, kShortLimitExceeded, kLongLimitExceeded };
- // Periods of the Font Display Timeline.
- // https://drafts.csswg.org/css-fonts-4/#font-display-timeline
- enum DisplayPeriod { kBlockPeriod, kSwapPeriod, kFailurePeriod };
RemoteFontFaceSource(CSSFontFace*, FontSelector*, FontDisplay);
~RemoteFontFaceSource() override;
@@ -60,6 +58,19 @@
const FontDescription&);
private:
+ // Periods of the Font Display Timeline.
+ // https://drafts.csswg.org/css-fonts-4/#font-display-timeline
+ // Note that kNotApplicablePeriod is an implementation detail indicating that
+ // the font is loaded from memory cache synchronously, and hence, made
+ // immediately available. As we never need to use a fallback for it, using
+ // other DisplayPeriod values seem artificial. So we use a special value.
+ enum DisplayPeriod {
+ kBlockPeriod,
+ kSwapPeriod,
+ kFailurePeriod,
+ kNotApplicablePeriod
+ };
+
class FontLoadHistograms {
DISALLOW_NEW();
@@ -113,6 +124,9 @@
DataSource data_source_;
};
+ Document* GetDocument() const;
+
+ DisplayPeriod ComputePeriod() const;
void UpdatePeriod();
bool ShouldTriggerWebFontsIntervention();
bool IsLowPriorityLoadingAllowedForRemoteFont() const override;
@@ -132,6 +146,7 @@
DisplayPeriod period_;
FontLoadHistograms histograms_;
bool is_intervention_triggered_;
+ bool finished_before_document_rendering_begin_;
};
} // namespace blink
diff --git a/third_party/blink/renderer/core/loader/font_preload_manager.cc b/third_party/blink/renderer/core/loader/font_preload_manager.cc
index 1ace308..fb88635 100644
--- a/third_party/blink/renderer/core/loader/font_preload_manager.cc
+++ b/third_party/blink/renderer/core/loader/font_preload_manager.cc
@@ -111,9 +111,6 @@
state_ = State::kUnblocked;
finish_observers_.clear();
-
- // TODO(xiaochengh): Mark all 'font-display: optional' fonts that are still
- // loading as failure.
}
void FontPreloadManager::FontPreloadingDelaysRenderingTimerFired(TimerBase*) {
diff --git a/third_party/blink/renderer/core/loader/font_preload_manager.h b/third_party/blink/renderer/core/loader/font_preload_manager.h
index 2279550..5d779ee 100644
--- a/third_party/blink/renderer/core/loader/font_preload_manager.h
+++ b/third_party/blink/renderer/core/loader/font_preload_manager.h
@@ -31,6 +31,7 @@
bool HasPendingRenderBlockingFonts() const;
void WillBeginRendering();
+ bool RenderingHasBegun() const { return state_ == State::kUnblocked; }
void FontPreloadingStarted(FontResource*);
void FontPreloadingFinished(FontResource*, ResourceFinishObserver*);
diff --git a/third_party/blink/renderer/core/loader/font_preload_manager_test.cc b/third_party/blink/renderer/core/loader/font_preload_manager_test.cc
index 2907254..feb4318 100644
--- a/third_party/blink/renderer/core/loader/font_preload_manager_test.cc
+++ b/third_party/blink/renderer/core/loader/font_preload_manager_test.cc
@@ -6,7 +6,11 @@
#include "base/test/scoped_feature_list.h"
#include "third_party/blink/public/common/features.h"
+#include "third_party/blink/renderer/core/dom/element.h"
#include "third_party/blink/renderer/core/frame/web_local_frame_impl.h"
+#include "third_party/blink/renderer/core/html/html_head_element.h"
+#include "third_party/blink/renderer/core/layout/layout_object.h"
+#include "third_party/blink/renderer/core/style/computed_style.h"
#include "third_party/blink/renderer/core/testing/sim/sim_request.h"
#include "third_party/blink/renderer/core/testing/sim/sim_test.h"
#include "third_party/blink/renderer/platform/testing/unit_test_helpers.h"
@@ -21,6 +25,11 @@
SimTest::SetUp();
}
+ static Vector<char> ReadAhemWoff2() {
+ return test::ReadFromFile(test::CoreTestDataPath("Ahem.woff2"))
+ ->CopyAs<Vector<char>>();
+ }
+
protected:
FontPreloadManager& GetFontPreloadManager() {
return GetDocument().GetFontPreloadManager();
@@ -29,6 +38,12 @@
using State = FontPreloadManager::State;
State GetState() { return GetFontPreloadManager().state_; }
+ Element* GetTarget() { return GetDocument().getElementById("target"); }
+
+ const Font& GetTargetFont() {
+ return GetTarget()->GetLayoutObject()->Style()->GetFont();
+ }
+
private:
base::test::ScopedFeatureList scoped_feature_list_;
};
@@ -170,4 +185,202 @@
font_resource.Finish();
}
+// A trivial test case to verify test setup
+TEST_F(FontPreloadManagerTest, RegularWebFont) {
+ SimRequest main_resource("https://example.com", "text/html");
+ SimRequest font_resource("https://example.com/Ahem.woff2", "font/woff2");
+
+ LoadURL("https://example.com");
+ main_resource.Complete(R"HTML(
+ <!doctype html>
+ <style>
+ @font-face {
+ font-family: custom-font;
+ src: url(https://example.com/Ahem.woff2) format("woff2");
+ }
+ #target {
+ font: 25px/1 custom-font, monospace;
+ }
+ </style>
+ <span id=target style="position:relative">0123456789</span>
+ )HTML");
+
+ // Now rendering has started, as there's no blocking resources.
+ EXPECT_FALSE(Compositor().DeferMainFrameUpdate());
+ EXPECT_EQ(State::kUnblocked, GetState());
+
+ font_resource.Complete(ReadAhemWoff2());
+
+ // Now everything is loaded. The web font should be used in rendering.
+ Compositor().BeginFrame().DrawCount();
+ EXPECT_EQ(250, GetTarget()->OffsetWidth());
+ EXPECT_FALSE(GetTargetFont().ShouldSkipDrawing());
+}
+
+TEST_F(FontPreloadManagerTest, OptionalFontWithoutPreloading) {
+ SimRequest main_resource("https://example.com", "text/html");
+ SimRequest font_resource("https://example.com/Ahem.woff2", "font/woff2");
+
+ LoadURL("https://example.com");
+ main_resource.Complete(R"HTML(
+ <!doctype html>
+ <style>
+ @font-face {
+ font-family: custom-font;
+ src: url(https://example.com/Ahem.woff2) format("woff2");
+ font-display: optional;
+ }
+ #target {
+ font: 25px/1 custom-font, monospace;
+ }
+ </style>
+ <span id=target>0123456789</span>
+ )HTML");
+
+ // Now rendering has started, as there's no blocking resources.
+ EXPECT_FALSE(Compositor().DeferMainFrameUpdate());
+ EXPECT_EQ(State::kUnblocked, GetState());
+
+ font_resource.Complete(ReadAhemWoff2());
+
+ // The 'optional' web font isn't used, as it didn't finish loading before
+ // rendering started. Text is rendered in visible fallback.
+ Compositor().BeginFrame().Contains(SimCanvas::kText);
+ EXPECT_GT(250, GetTarget()->OffsetWidth());
+ EXPECT_FALSE(GetTargetFont().ShouldSkipDrawing());
+}
+
+TEST_F(FontPreloadManagerTest, OptionalFontRemoveAndReadd) {
+ SimRequest main_resource("https://example.com", "text/html");
+ SimRequest font_resource("https://example.com/Ahem.woff2", "font/woff2");
+
+ LoadURL("https://example.com");
+ main_resource.Complete(R"HTML(
+ <!doctype html>
+ <style>
+ @font-face {
+ font-family: custom-font;
+ src: url(https://example.com/Ahem.woff2) format("woff2");
+ font-display: optional;
+ }
+ #target {
+ font: 25px/1 custom-font, monospace;
+ }
+ </style>
+ <span id=target>0123456789</span>
+ )HTML");
+
+ // Now rendering has started, as there's no blocking resources.
+ EXPECT_FALSE(Compositor().DeferMainFrameUpdate());
+ EXPECT_EQ(State::kUnblocked, GetState());
+
+ // The 'optional' web font isn't used, as it didn't finish loading before
+ // rendering started. Text is rendered in visible fallback.
+ Compositor().BeginFrame().Contains(SimCanvas::kText);
+ EXPECT_GT(250, GetTarget()->OffsetWidth());
+ EXPECT_FALSE(GetTargetFont().ShouldSkipDrawing());
+
+ font_resource.Complete(ReadAhemWoff2());
+
+ Element* style = GetDocument().QuerySelector("style");
+ style->remove();
+ GetDocument().head()->appendChild(style);
+
+ // After removing and readding the style sheet, we've created a new font face
+ // that got loaded immediately from the memory cache. So it can be used.
+ Compositor().BeginFrame().Contains(SimCanvas::kText);
+ EXPECT_EQ(250, GetTarget()->OffsetWidth());
+ EXPECT_FALSE(GetTargetFont().ShouldSkipDrawing());
+}
+
+TEST_F(FontPreloadManagerTest, OptionalFontSlowPreloading) {
+ SimRequest main_resource("https://example.com", "text/html");
+ SimRequest font_resource("https://example.com/Ahem.woff2", "font/woff2");
+
+ LoadURL("https://example.com");
+ main_resource.Complete(R"HTML(
+ <!doctype html>
+ <link rel="preload" as="font" type="font/woff2"
+ href="https://example.com/Ahem.woff2" crossorigin>
+ <style>
+ @font-face {
+ font-family: custom-font;
+ src: url(https://example.com/Ahem.woff2) format("woff2");
+ font-display: optional;
+ }
+ #target {
+ font: 25px/1 custom-font, monospace;
+ }
+ </style>
+ <span id=target>0123456789</span>
+ )HTML");
+
+ // Rendering is blocked due to font being preloaded.
+ EXPECT_TRUE(Compositor().DeferMainFrameUpdate());
+ EXPECT_TRUE(GetFontPreloadManager().HasPendingRenderBlockingFonts());
+ EXPECT_EQ(State::kLoading, GetState());
+
+ GetFontPreloadManager().FontPreloadingDelaysRenderingTimerFired(nullptr);
+
+ // Rendering is unblocked after the font preloading has timed out.
+ EXPECT_FALSE(Compositor().DeferMainFrameUpdate());
+ EXPECT_FALSE(GetFontPreloadManager().HasPendingRenderBlockingFonts());
+ EXPECT_EQ(State::kUnblocked, GetState());
+
+ // First frame renders text with visible fallback, as the 'optional' web font
+ // isn't loaded yet, and should be treated as in the failure period.
+ Compositor().BeginFrame();
+ EXPECT_GT(250, GetTarget()->OffsetWidth());
+ EXPECT_FALSE(GetTargetFont().ShouldSkipDrawing());
+
+ font_resource.Complete(ReadAhemWoff2());
+
+ // The 'optional' web font should not cause relayout even if it finishes
+ // loading now.
+ Compositor().BeginFrame();
+ EXPECT_GT(250, GetTarget()->OffsetWidth());
+ EXPECT_FALSE(GetTargetFont().ShouldSkipDrawing());
+}
+
+TEST_F(FontPreloadManagerTest, OptionalFontFastPreloading) {
+ SimRequest main_resource("https://example.com", "text/html");
+ SimRequest font_resource("https://example.com/Ahem.woff2", "font/woff2");
+
+ LoadURL("https://example.com");
+ main_resource.Complete(R"HTML(
+ <!doctype html>
+ <link rel="preload" as="font" type="font/woff2"
+ href="https://example.com/Ahem.woff2" crossorigin>
+ <style>
+ @font-face {
+ font-family: custom-font;
+ src: url(https://example.com/Ahem.woff2) format("woff2");
+ font-display: optional;
+ }
+ #target {
+ font: 25px/1 custom-font, monospace;
+ }
+ </style>
+ <span id=target>0123456789</span>
+ )HTML");
+
+ // Rendering is blocked due to font being preloaded.
+ EXPECT_TRUE(Compositor().DeferMainFrameUpdate());
+ EXPECT_TRUE(GetFontPreloadManager().HasPendingRenderBlockingFonts());
+ EXPECT_EQ(State::kLoading, GetState());
+
+ font_resource.Complete(ReadAhemWoff2());
+ test::RunPendingTasks();
+
+ // Rendering is unblocked after the font is preloaded.
+ EXPECT_FALSE(Compositor().DeferMainFrameUpdate());
+ EXPECT_FALSE(GetFontPreloadManager().HasPendingRenderBlockingFonts());
+ EXPECT_EQ(State::kUnblocked, GetState());
+
+ // The 'optional' web font should be used in the first paint.
+ Compositor().BeginFrame();
+ EXPECT_EQ(250, GetTarget()->OffsetWidth());
+ EXPECT_FALSE(GetTargetFont().ShouldSkipDrawing());
+}
+
} // namespace blink
diff --git a/third_party/blink/renderer/core/testing/data/Ahem.woff2 b/third_party/blink/renderer/core/testing/data/Ahem.woff2
new file mode 100644
index 0000000..bf3cc77
--- /dev/null
+++ b/third_party/blink/renderer/core/testing/data/Ahem.woff2
Binary files differ
diff --git a/third_party/blink/renderer/platform/loader/fetch/resource.cc b/third_party/blink/renderer/platform/loader/fetch/resource.cc
index 2f7d7eb..f998c66 100644
--- a/third_party/blink/renderer/platform/loader/fetch/resource.cc
+++ b/third_party/blink/renderer/platform/loader/fetch/resource.cc
@@ -567,22 +567,23 @@
return builder.ToString();
}
-void Resource::DidAddClient(ResourceClient* c) {
+void Resource::DidAddClient(ResourceClient* client) {
if (scoped_refptr<SharedBuffer> data = Data()) {
for (const auto& span : *data) {
- c->DataReceived(this, span.data(), span.size());
+ client->DataReceived(this, span.data(), span.size());
// Stop pushing data if the client removed itself.
- if (!HasClient(c))
+ if (!HasClient(client))
break;
}
}
- if (!HasClient(c))
+ if (!HasClient(client))
return;
if (IsFinishedInternal()) {
- c->NotifyFinished(this);
- if (clients_.Contains(c)) {
- finished_clients_.insert(c);
- clients_.erase(c);
+ client->SetHasFinishedFromMemoryCache();
+ client->NotifyFinished(this);
+ if (clients_.Contains(client)) {
+ finished_clients_.insert(client);
+ clients_.erase(client);
}
}
}
diff --git a/third_party/blink/renderer/platform/loader/fetch/resource_client.h b/third_party/blink/renderer/platform/loader/fetch/resource_client.h
index dced85b..a4ea28e 100644
--- a/third_party/blink/renderer/platform/loader/fetch/resource_client.h
+++ b/third_party/blink/renderer/platform/loader/fetch/resource_client.h
@@ -64,6 +64,9 @@
Resource* GetResource() const { return resource_; }
+ bool FinishedFromMemoryCache() const { return finished_from_memory_cache_; }
+ void SetHasFinishedFromMemoryCache() { finished_from_memory_cache_ = true; }
+
// Name for debugging, e.g. shown in memory-infra.
virtual String DebugName() const = 0;
@@ -86,6 +89,10 @@
base::SingleThreadTaskRunner* task_runner);
Member<Resource> resource_;
+
+ // If true, the Resource was already available from the memory cache when this
+ // ResourceClient was setup, so that the request finished immediately.
+ bool finished_from_memory_cache_ = false;
};
} // namespace blink
