Skip to content

Commit

Permalink
[M102] Fix a regression that CascadeLayerMap is not rebuilt
Browse files Browse the repository at this point in the history
A previous patch [1] introduced a regression that CascadeLayerMap is not
rebuilt if (1) previously there are no layers, and (2) layers are added.

This patch fixes it by making sure that CascadeLayerMap is always
rebuilt if there are layers in the new active style sheets, including
two cases (which are not necessarily disjoint):
1. Before updating active style, there are already layers
2. There are layers in the changed rule sets

[1] https://chromium-review.googlesource.com/c/chromium/src/+/3572544

(cherry picked from commit 523fd78)

(cherry picked from commit 6e52b8e)

Fixed: 1326791
Change-Id: I05d2aa9dbc9b69d6fbf26abd9a74f27fc3c8aec7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3656429
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Auto-Submit: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Original-Original-Commit-Position: refs/heads/main@{#1005842}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3659435
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Original-Commit-Position: refs/branch-heads/5060@{#170}
Cr-Original-Branched-From: b83393d-refs/heads/main@{#1002911}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3683895
Cr-Commit-Position: refs/branch-heads/5005@{#1081}
Cr-Branched-From: 5b4d945-refs/heads/main@{#992738}
  • Loading branch information
xiaochengh authored and Chromium LUCI CQ committed Jun 1, 2022
1 parent 93d466f commit 2eabf9b
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 4 deletions.
4 changes: 2 additions & 2 deletions third_party/blink/renderer/core/css/css_property_value_set.cc
Expand Up @@ -322,8 +322,8 @@ bool CSSPropertyValueSet::PropertyIsImportant(const T& property) const {
return PropertyAt(found_property_index).IsImportant();
return ShorthandIsImportant(property);
}
template bool CSSPropertyValueSet::PropertyIsImportant<CSSPropertyID>(
const CSSPropertyID&) const;
template CORE_EXPORT bool CSSPropertyValueSet::PropertyIsImportant<
CSSPropertyID>(const CSSPropertyID&) const;
template bool CSSPropertyValueSet::PropertyIsImportant<AtomicString>(
const AtomicString&) const;

Expand Down
Expand Up @@ -1600,6 +1600,54 @@ TEST_F(StyleResolverTest, CascadeLayersAfterModifyingAnotherSheet) {
EXPECT_EQ(properties[1].types_.origin, CascadeOrigin::kAuthor);
}

// https://crbug.com/1326791
TEST_F(StyleResolverTest, CascadeLayersAddLayersWithImportantDeclarations) {
GetDocument().documentElement()->setInnerHTML(R"HTML(
<style id="addrule"></style>
<target></target>
)HTML");

UpdateAllLifecyclePhasesForTest();

GetDocument().getElementById("addrule")->appendChild(
GetDocument().createTextNode(
"@layer { target { font-size: 20px !important; } }"
"@layer { target { font-size: 10px !important; } }"));

UpdateAllLifecyclePhasesForTest();

ASSERT_TRUE(GetDocument().GetScopedStyleResolver()->GetCascadeLayerMap());

StyleResolverState state(GetDocument(),
*GetDocument().QuerySelector("target"));
SelectorFilter filter;
MatchResult match_result;
ElementRuleCollector collector(state.ElementContext(), StyleRecalcContext(),
filter, match_result, state.Style(),
EInsideLink::kNotInsideLink);
MatchAllRules(state, collector);
const auto& properties = match_result.GetMatchedProperties();
ASSERT_EQ(properties.size(), 2u);

// @layer { target { font-size: 20px !important } }
EXPECT_TRUE(properties[0].properties->HasProperty(CSSPropertyID::kFontSize));
EXPECT_TRUE(
properties[0].properties->PropertyIsImportant(CSSPropertyID::kFontSize));
EXPECT_EQ("20px", properties[0].properties->GetPropertyValue(
CSSPropertyID::kFontSize));
EXPECT_EQ(0u, properties[0].types_.layer_order);
EXPECT_EQ(properties[0].types_.origin, CascadeOrigin::kAuthor);

// @layer { target { font-size: 10px !important } }
EXPECT_TRUE(properties[1].properties->HasProperty(CSSPropertyID::kFontSize));
EXPECT_TRUE(
properties[1].properties->PropertyIsImportant(CSSPropertyID::kFontSize));
EXPECT_EQ("10px", properties[1].properties->GetPropertyValue(
CSSPropertyID::kFontSize));
EXPECT_EQ(1u, properties[1].types_.layer_order);
EXPECT_EQ(properties[1].types_.origin, CascadeOrigin::kAuthor);
}

// TODO(crbug.com/1095765): We should have a WPT for this test case, but
// currently Blink web test runner can't test @page rules in WPT.
TEST_F(StyleResolverTest, CascadeLayersAndPageRules) {
Expand Down
5 changes: 3 additions & 2 deletions third_party/blink/renderer/core/css/style_engine.cc
Expand Up @@ -2054,14 +2054,15 @@ void StyleEngine::ApplyRuleSetChanges(
// - If new sheets were appended to existing ones, start appending after the
// common prefix, and rebuild CascadeLayerMap only if layers are changed.
// - For other diffs, reset author style and re-add all sheets for the
// TreeScope. If there is an existing CascadeLayerMap, rebuild it.
// TreeScope. If new sheets need a CascadeLayerMap, rebuild it.
if (new_style_sheets.IsEmpty()) {
rebuild_cascade_layer_map = false;
ResetAuthorStyle(tree_scope);
} else if (change == kActiveSheetsAppended) {
append_start_index = old_style_sheets.size();
} else {
rebuild_cascade_layer_map = scoped_resolver->HasCascadeLayerMap();
rebuild_cascade_layer_map = (changed_rule_flags & kLayerRules) ||
scoped_resolver->HasCascadeLayerMap();
scoped_resolver->ResetStyle();
}
}
Expand Down

0 comments on commit 2eabf9b

Please sign in to comment.