Skip to content

Commit

Permalink
[StyleBuilder] Stop decorations relying on ComputedStyleBuilder::style_
Browse files Browse the repository at this point in the history
This patch changes how text decorations are calculated an applied.

Previously the AppliedTextDecorations() field was inherited/applied,
then within the StyleAdjuster, ApplyTextDecorations() was called
which looked at this inherited field, and mutated it adding an
additional decoration if needed.

ApplyTextDecorations needed access to style_ to resolve the color of decorations.

This patch changes inherits the "parent" text decorations, then
computed the actual decorations after the style has been built.

It does this via an AppliedTextDecorations vector on the
StyleCachedData object.

Bug: 1377295
Change-Id: Ib4000af23caebc9f37e40862b521b17b355d2c23
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4234822
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1106648}
  • Loading branch information
bfgeek authored and Chromium LUCI CQ committed Feb 17, 2023
1 parent 3a880f0 commit 6dd95a6
Show file tree
Hide file tree
Showing 16 changed files with 121 additions and 205 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,15 @@ def __init__(self, json5_file_paths, output_dir):
self._forward_declarations = set()

for property_ in self._properties:
# TODO(meade): CursorList and AppliedTextDecorationList are
# typedefs, not classes, so they can't be forward declared. Find a
# better way to specify this.
# TODO(meade): CursorList is a typedef, not a class, so it can't be
# forward declared. Find a better way to specify this.
# Omitting template classes because they can't be forward-declared
# when they're instantiated. TODO: parse/treat them correctly so
# they can be forward-declared.
# TODO: check that the class that is being forward-declared is not
# among self._includes as this could make the compiler throw errors.
if property_.default_value == 'nullptr' \
and not property_.unwrapped_type_name == 'CursorList' \
and not property_.unwrapped_type_name == \
'AppliedTextDecorationList' and \
and not property_.unwrapped_type_name == 'CursorList' and \
self.is_not_template_class(property_.unwrapped_type_name):
self._forward_declarations.add(property_.unwrapped_type_name)
else:
Expand Down
12 changes: 4 additions & 8 deletions third_party/blink/renderer/core/css/resolver/style_adjuster.cc
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ void StyleAdjuster::AdjustStyleForCombinedText(ComputedStyleBuilder& builder) {
builder.SetWordSpacing(0.0f);
builder.SetWritingMode(WritingMode::kHorizontalTb);

builder.ClearAppliedTextDecorations();
builder.SetBaseTextDecorationData(nullptr);
builder.ResetTextIndent();
builder.UpdateFontOrientation();

Expand Down Expand Up @@ -943,9 +943,10 @@ void StyleAdjuster::AdjustComputedStyle(StyleResolverState& state,
// Highlight pseudos propagate decorations with inheritance only.
if (StopPropagateTextDecorations(builder, element) ||
state.IsForHighlight()) {
builder.ClearAppliedTextDecorations();
builder.SetBaseTextDecorationData(nullptr);
} else {
builder.RestoreParentTextDecorations(layout_parent_style);
builder.SetBaseTextDecorationData(
layout_parent_style.AppliedTextDecorationData());
}

// The computed value of currentColor for highlight pseudos is the
Expand All @@ -963,11 +964,6 @@ void StyleAdjuster::AdjustComputedStyle(StyleResolverState& state,
}
}

if (builder.Display() != EDisplay::kContents) {
builder.ApplyTextDecorations(parent_style.VisitedDependentColorFast(
GetCSSPropertyTextDecorationColor()));
}

// Cull out any useless layers and also repeat patterns into additional
// layers.
builder.AdjustBackgroundLayers();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ LayoutRect NGInkOverflow::ComputeTextDecorationOverflow(
nullptr, &scaled_font, kMinimumThicknessIsOne);
NGTextDecorationOffset decoration_offset(decoration_info.TargetStyle(),
style);
const Vector<AppliedTextDecoration>& decorations =
const Vector<AppliedTextDecoration, 1>& decorations =
style.AppliedTextDecorations();

gfx::RectF accumulated_bound;
Expand Down
8 changes: 4 additions & 4 deletions third_party/blink/renderer/core/paint/ng/ng_decorating_box.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class CORE_EXPORT NGDecoratingBox {
public:
NGDecoratingBox(const PhysicalOffset& content_offset_in_container,
const ComputedStyle& style,
const Vector<AppliedTextDecoration>* decorations)
const Vector<AppliedTextDecoration, 1>* decorations)
: content_offset_in_container_(content_offset_in_container),
style_(&style),
decorations_(decorations ? decorations
Expand All @@ -30,7 +30,7 @@ class CORE_EXPORT NGDecoratingBox {
}
NGDecoratingBox(const NGFragmentItem& item,
const ComputedStyle& style,
const Vector<AppliedTextDecoration>* decorations)
const Vector<AppliedTextDecoration, 1>* decorations)
: NGDecoratingBox(item.ContentOffsetInContainerFragment(),
style,
decorations) {}
Expand All @@ -41,14 +41,14 @@ class CORE_EXPORT NGDecoratingBox {
return content_offset_in_container_;
}
const ComputedStyle& Style() const { return *style_; }
const Vector<AppliedTextDecoration>* AppliedTextDecorations() const {
const Vector<AppliedTextDecoration, 1>* AppliedTextDecorations() const {
return decorations_;
}

private:
PhysicalOffset content_offset_in_container_;
const ComputedStyle* style_;
const Vector<AppliedTextDecoration>* decorations_;
const Vector<AppliedTextDecoration, 1>* decorations_;
};

} // namespace blink
Expand Down
21 changes: 11 additions & 10 deletions third_party/blink/renderer/core/paint/ng/ng_inline_paint_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ wtf_size_t NGInlinePaintContext::SyncDecoratingBox(
// Compare the instance addresses of |AppliedTextDecorations| because it is
// shared across |ComputedStyle|s when it is propagated without changes.
const ComputedStyle* style = &item.Style();
const Vector<AppliedTextDecoration>* decorations =
const Vector<AppliedTextDecoration, 1>* decorations =
&style->AppliedTextDecorations();
DCHECK(last_decorations_);
if (decorations == last_decorations_)
Expand All @@ -60,7 +60,7 @@ wtf_size_t NGInlinePaintContext::SyncDecoratingBox(
public:
DecorationBoxSynchronizer(NGInlinePaintContext* inline_context,
const NGFragmentItem& item,
const Vector<AppliedTextDecoration>* stop_at,
const Vector<AppliedTextDecoration, 1>* stop_at,
DecoratingBoxList* saved_decorating_boxes)
: inline_context_(inline_context),
stop_at_(stop_at),
Expand All @@ -73,7 +73,7 @@ wtf_size_t NGInlinePaintContext::SyncDecoratingBox(
wtf_size_t Sync(const NGFragmentItem* item,
const LayoutObject* layout_object,
const ComputedStyle* style,
const Vector<AppliedTextDecoration>* decorations) {
const Vector<AppliedTextDecoration, 1>* decorations) {
for (;;) {
DCHECK(!item || item->GetLayoutObject() == layout_object);
DCHECK_EQ(&layout_object->EffectiveStyle(style_variant_), style);
Expand All @@ -83,7 +83,7 @@ wtf_size_t NGInlinePaintContext::SyncDecoratingBox(
DCHECK(parent);
const ComputedStyle& parent_style =
parent->EffectiveStyle(style_variant_);
const Vector<AppliedTextDecoration>& parent_decorations =
const Vector<AppliedTextDecoration, 1>& parent_decorations =
parent_style.AppliedTextDecorations();

if (decorations != &parent_decorations) {
Expand Down Expand Up @@ -172,10 +172,11 @@ wtf_size_t NGInlinePaintContext::SyncDecoratingBox(
}
}

void PushDecoratingBox(const NGFragmentItem* item,
const LayoutObject& layout_object,
const ComputedStyle& style,
const Vector<AppliedTextDecoration>& decorations) {
void PushDecoratingBox(
const NGFragmentItem* item,
const LayoutObject& layout_object,
const ComputedStyle& style,
const Vector<AppliedTextDecoration, 1>& decorations) {
DCHECK(!item || item->GetLayoutObject() == &layout_object);
if (!item) {
// If the item is not known, it is either a culled inline or it is found
Expand All @@ -193,7 +194,7 @@ wtf_size_t NGInlinePaintContext::SyncDecoratingBox(
}

NGInlinePaintContext* inline_context_;
const Vector<AppliedTextDecoration>* stop_at_;
const Vector<AppliedTextDecoration, 1>* stop_at_;
absl::optional<NGInlineCursor> line_cursor_;
DecoratingBoxList* saved_decorating_boxes_;
NGStyleVariant style_variant_;
Expand Down Expand Up @@ -260,7 +261,7 @@ void NGInlinePaintContext::SetLineBox(const NGInlineCursor& line_cursor) {

const NGFragmentItem& line_item = *line_cursor.Current();
const ComputedStyle& style = line_item.Style();
const Vector<AppliedTextDecoration>& applied_text_decorations =
const Vector<AppliedTextDecoration, 1>& applied_text_decorations =
style.AppliedTextDecorations();
line_decorations_ = last_decorations_ = &applied_text_decorations;
if (applied_text_decorations.empty())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class CORE_EXPORT NGInlinePaintContext {

private:
NGInlinePaintContext* inline_context_ = nullptr;
const Vector<AppliedTextDecoration>* last_decorations_ = nullptr;
const Vector<AppliedTextDecoration, 1>* last_decorations_ = nullptr;
DecoratingBoxList saved_decorating_boxes_;
wtf_size_t push_count_ = 0;
};
Expand Down Expand Up @@ -105,8 +105,8 @@ class CORE_EXPORT NGInlinePaintContext {

DecoratingBoxList decorating_boxes_;
// The last |AppliedTextDecorations| |this| was synchronized with.
const Vector<AppliedTextDecoration>* last_decorations_ = nullptr;
const Vector<AppliedTextDecoration>* line_decorations_ = nullptr;
const Vector<AppliedTextDecoration, 1>* last_decorations_ = nullptr;
const Vector<AppliedTextDecoration, 1>* line_decorations_ = nullptr;
absl::optional<NGInlineCursor> line_cursor_;
PhysicalOffset paint_offset_;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ void SVGInlineTextBoxPainter::PaintTextFragments(
// Spec: All text decorations except line-through should be drawn before the
// text is filled and stroked; thus, the text is rendered on top of these
// decorations.
const Vector<AppliedTextDecoration>& decorations =
const Vector<AppliedTextDecoration, 1>& decorations =
style.AppliedTextDecorations();
for (const AppliedTextDecoration& decoration : decorations) {
if (EnumHasFlags(decoration.Lines(), TextDecorationLine::kUnderline))
Expand Down
4 changes: 2 additions & 2 deletions third_party/blink/renderer/core/paint/text_painter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ void TextPainter::PaintDecorationsExceptLineThrough(
const TextDecorationOffsetBase& decoration_offset,
TextDecorationInfo& decoration_info,
const PaintInfo& paint_info,
const Vector<AppliedTextDecoration>& decorations,
const Vector<AppliedTextDecoration, 1>& decorations,
const TextPaintStyle& text_style) {
// Updating the graphics context and looping through applied decorations is
// expensive, so avoid doing it if the only decoration was a ‘line-through’.
Expand Down Expand Up @@ -116,7 +116,7 @@ void TextPainter::PaintDecorationsExceptLineThrough(
void TextPainter::PaintDecorationsOnlyLineThrough(
TextDecorationInfo& decoration_info,
const PaintInfo& paint_info,
const Vector<AppliedTextDecoration>& decorations,
const Vector<AppliedTextDecoration, 1>& decorations,
const TextPaintStyle& text_style) {
// Updating the graphics context and looping through applied decorations is
// expensive, so avoid doing it if there are no ‘line-through’ decorations.
Expand Down
13 changes: 7 additions & 6 deletions third_party/blink/renderer/core/paint/text_painter.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,15 @@ class CORE_EXPORT TextPainter : public TextPainterBase {
DOMNodeId node_id,
const AutoDarkMode& auto_dark_mode);

void PaintDecorationsExceptLineThrough(const TextDecorationOffsetBase&,
TextDecorationInfo&,
const PaintInfo&,
const Vector<AppliedTextDecoration>&,
const TextPaintStyle& text_style);
void PaintDecorationsExceptLineThrough(
const TextDecorationOffsetBase&,
TextDecorationInfo&,
const PaintInfo&,
const Vector<AppliedTextDecoration, 1>&,
const TextPaintStyle& text_style);
void PaintDecorationsOnlyLineThrough(TextDecorationInfo&,
const PaintInfo&,
const Vector<AppliedTextDecoration>&,
const Vector<AppliedTextDecoration, 1>&,
const TextPaintStyle&);

private:
Expand Down

This file was deleted.

1 change: 0 additions & 1 deletion third_party/blink/renderer/core/style/build.gni
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ blink_core_sources_style = [
"anchor_specifier_value.h",
"applied_text_decoration.cc",
"applied_text_decoration.h",
"applied_text_decoration_list.h",
"basic_shapes.cc",
"basic_shapes.h",
"border_edge.cc",
Expand Down

0 comments on commit 6dd95a6

Please sign in to comment.