Commit fa8d19fe authored by Koji Ishii's avatar Koji Ishii Committed by Commit Bot

Test NeedsCollectInlines for text-decorations and outline changes

The purpose of this test is to ensure |NeedsCollectInlines|
is not set by `text-decoration-line`, because it is one of the
most expensive operations, and the property is often changed
dynamically (e.g., hover on links.)

r807433 crrev.com/c/2411300 changed it to test
`text-decoration-color` instead, because it changed
`text-decoration-line` to invalidate visual overflow, and
because crbug.com/1043927 added a workaround to
|SetNeedsLayoutAndIntrinsicWidthsRecalc| when
|NeedsRecomputeVisualOverflow| for NG inline objects.

This patch changes the test to:
* Change to test `text-decoration-line` property.
* Change to expect |NeedsLayout| is set, with TODO comment.
* Add `outline` property, another property commonly changed
  dynamically (e.g., focus ring.)

Bug: 1128199, 1043927
Change-Id: I879582916d45a9374fada4e8d42f5baca95adabb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2423766Reviewed-by: default avatarStephen Chenney <schenney@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809728}
parent dcda8c2e
......@@ -2319,11 +2319,11 @@ void LayoutObject::SetStyle(scoped_refptr<const ComputedStyle> style,
if (diff.NeedsRecomputeVisualOverflow()) {
if (!IsLayoutNGObject() && !IsLayoutBlock() && !NeedsLayout()) {
// TODO(rego): This is still needed because RecalcVisualOverflow() does
// not actually compute the visual overflow for inline elements (legacy
// layout). However in LayoutNG RecalcInlineChildrenInkOverflow() is
// called and visual overflow is recomputed properly so we don't need this
// (see crbug.com/1043927).
// TODO(crbug.com/1128199): This is still needed because
// RecalcVisualOverflow() does not actually compute the visual overflow
// for inline elements (legacy layout). However in LayoutNG
// RecalcInlineChildrenInkOverflow() is called and visual overflow is
// recomputed properly so we don't need this (see crbug.com/1043927).
SetNeedsLayoutAndIntrinsicWidthsRecalc(
layout_invalidation_reason::kStyleChange);
} else {
......
......@@ -141,7 +141,7 @@ class NGInlineNodeTest : public NGLayoutTest {
return end_offsets;
}
void TestAnyItrermsAreDirty(LayoutBlockFlow* block_flow, bool expected) {
void TestAnyItemsAreDirty(LayoutBlockFlow* block_flow, bool expected) {
const NGFragmentItems* items = block_flow->FragmentItems();
items->DirtyLinesFromNeedsLayout(block_flow);
// Check |NGFragmentItem::IsDirty| directly without using
......@@ -599,11 +599,14 @@ struct StyleChangeData {
unsigned needs_collect_inlines;
base::Optional<bool> is_line_dirty;
} style_change_data[] = {
// Changing color, etc. should not re-run
// Changing color, text-decoration, outline, etc. should not re-run
// |CollectInlines()|.
{"#parent.after { color: red; }", StyleChangeData::kNone, false},
{"#parent.after { text-decoration-color: red; }", StyleChangeData::kNone,
false},
// TODO(crbug.com/1128199): text-decorations, outline, etc. should not
// require layout, only ink overflow, but they currently do.
{"#parent.after { text-decoration-line: underline; }",
StyleChangeData::kNone, true},
{"#parent.after { outline: auto; }", StyleChangeData::kNone, true},
// Changing fonts should re-run |CollectInlines()|.
{"#parent.after { font-size: 200%; }", StyleChangeData::kAll, true},
// Changing from/to out-of-flow should re-rerun |CollectInlines()|.
......@@ -682,8 +685,8 @@ TEST_P(StyleChangeTest, NeedsCollectInlinesOnStyle) {
if (data.is_line_dirty &&
RuntimeEnabledFeatures::LayoutNGFragmentItemEnabled()) {
TestAnyItrermsAreDirty(To<LayoutBlockFlow>(container->GetLayoutObject()),
*data.is_line_dirty);
TestAnyItemsAreDirty(To<LayoutBlockFlow>(container->GetLayoutObject()),
*data.is_line_dirty);
}
ForceLayout(); // Ensure running layout does not crash.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment