Skip to content

Commit

Permalink
[StyleBuilder] Make top/right/bottom/left properties readonly.
Browse files Browse the repository at this point in the history 
To do this we need to introduce the builder for the menu-list popup,
and clone the original style, instead of mutating it.

There should be no behaviour change.

Bug: 1377295
Change-Id: Ic1f940d513246984a2c874be986b2063712d7d16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3988190
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1064929}
  • Loading branch information
@bfgeek
bfgeek authored and Chromium LUCI CQ committed Oct 28, 2022
1 parent a32148f commit 41e0f1e
Show file tree
Hide file tree
Showing 6 changed files with 94 additions and 84 deletions.
8 changes: 4 additions & 4 deletions third_party/blink/renderer/core/animation/length_property_functions.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ bool LengthPropertyFunctions::SetLength(const CSSProperty& property,
builder.SetBaselineShift(value);
return true;
case CSSPropertyID::kBottom:
style.SetBottom(value);
builder.SetBottom(value);
return true;
case CSSPropertyID::kCx:
builder.SetCx(value);
Expand All @@ -345,7 +345,7 @@ bool LengthPropertyFunctions::SetLength(const CSSProperty& property,
style.SetHeight(value);
return true;
case CSSPropertyID::kLeft:
style.SetLeft(value);
builder.SetLeft(value);
return true;
case CSSPropertyID::kMarginBottom:
style.SetMarginBottom(value);
Expand Down Expand Up @@ -396,7 +396,7 @@ bool LengthPropertyFunctions::SetLength(const CSSProperty& property,
builder.SetRy(value);
return true;
case CSSPropertyID::kRight:
style.SetRight(value);
builder.SetRight(value);
return true;
case CSSPropertyID::kShapeMargin:
style.SetShapeMargin(value);
Expand All @@ -405,7 +405,7 @@ bool LengthPropertyFunctions::SetLength(const CSSProperty& property,
builder.SetStrokeDashOffset(value);
return true;
case CSSPropertyID::kTop:
style.SetTop(value);
builder.SetTop(value);
return true;
case CSSPropertyID::kWidth:
style.SetWidth(value);
Expand Down
4 changes: 4 additions & 0 deletions third_party/blink/renderer/core/css/css_properties.json5
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2158,6 +2158,7 @@
},
supports_incremental_style: true,
valid_for_position_fallback: true,
readonly: true,
},
{
name: "box-shadow",
Expand Down Expand Up @@ -3028,6 +3029,7 @@
},
supports_incremental_style: true,
valid_for_position_fallback: true,
readonly: true,
},
{
name: "letter-spacing",
Expand Down Expand Up @@ -3992,6 +3994,7 @@
},
supports_incremental_style: true,
valid_for_position_fallback: true,
readonly: true,
},
{
name: "r",
Expand Down Expand Up @@ -4942,6 +4945,7 @@
},
supports_incremental_style: true,
valid_for_position_fallback: true,
readonly: true,
},
{
name: "touch-action",
Expand Down
48 changes: 24 additions & 24 deletions third_party/blink/renderer/core/css/resolver/style_adjuster.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,34 +135,34 @@ void AdjustStyleForSvgElement(const SVGElement& element, ComputedStyle& style) {
}

// Adjust style for anchor() and anchor-size() queries.
void AdjustAnchorQueryStyles(ComputedStyle& style) {
void AdjustAnchorQueryStyles(ComputedStyleBuilder& builder) {
if (!RuntimeEnabledFeatures::CSSAnchorPositioningEnabled())
return;

// anchor() and anchor-size() can only be used on absolutely positioned
// elements.
if (style.GetPosition() != EPosition::kAbsolute &&
style.GetPosition() != EPosition::kFixed) {
if (style.Left().HasAnchorQueries())
style.SetLeft(Length::Auto());
if (style.Right().HasAnchorQueries())
style.SetRight(Length::Auto());
if (style.Top().HasAnchorQueries())
style.SetTop(Length::Auto());
if (style.Bottom().HasAnchorQueries())
style.SetBottom(Length::Auto());
if (style.Width().HasAnchorQueries())
style.SetWidth(Length::Auto());
if (style.MinWidth().HasAnchorQueries())
style.SetMinWidth(Length::Auto());
if (style.MaxWidth().HasAnchorQueries())
style.SetMaxWidth(Length::Auto());
if (style.Height().HasAnchorQueries())
style.SetHeight(Length::Auto());
if (style.MinHeight().HasAnchorQueries())
style.SetMinHeight(Length::Auto());
if (style.MaxHeight().HasAnchorQueries())
style.SetMaxHeight(Length::Auto());
EPosition position = builder.InternalStyle()->GetPosition();
if (position != EPosition::kAbsolute && position != EPosition::kFixed) {
if (builder.Left().HasAnchorQueries())
builder.SetLeft(Length::Auto());
if (builder.Right().HasAnchorQueries())
builder.SetRight(Length::Auto());
if (builder.Top().HasAnchorQueries())
builder.SetTop(Length::Auto());
if (builder.Bottom().HasAnchorQueries())
builder.SetBottom(Length::Auto());
if (builder.Width().HasAnchorQueries())
builder.SetWidth(Length::Auto());
if (builder.MinWidth().HasAnchorQueries())
builder.SetMinWidth(Length::Auto());
if (builder.MaxWidth().HasAnchorQueries())
builder.SetMaxWidth(Length::Auto());
if (builder.Height().HasAnchorQueries())
builder.SetHeight(Length::Auto());
if (builder.MinHeight().HasAnchorQueries())
builder.SetMinHeight(Length::Auto());
if (builder.MaxHeight().HasAnchorQueries())
builder.SetMaxHeight(Length::Auto());
}
}

Expand Down Expand Up @@ -1074,7 +1074,7 @@ void StyleAdjuster::AdjustComputedStyle(StyleResolverState& state,
}
}

AdjustAnchorQueryStyles(style);
AdjustAnchorQueryStyles(builder);

if (!HasFullNGFragmentationSupport()) {
// When establishing a block fragmentation context for LayoutNG, we require
Expand Down
48 changes: 27 additions & 21 deletions third_party/blink/renderer/core/html/html_element.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1816,16 +1816,22 @@ void HTMLElement::SetOwnerSelectMenuElement(HTMLSelectMenuElement* element) {

// TODO(crbug.com/1197720): The popup position should be provided by the new
// anchored positioning scheme.
void HTMLElement::AdjustPopupPositionForSelectMenu(ComputedStyle& style) {
scoped_refptr<ComputedStyle> HTMLElement::StyleForSelectMenuPopupstyle(
const StyleRecalcContext& style_recalc_context) {
DCHECK(RuntimeEnabledFeatures::HTMLSelectMenuElementEnabled());
DCHECK(HasPopupAttribute());
DCHECK(GetPopupData()->needsRepositioningForSelectMenu());
auto* owner_select = GetPopupData()->ownerSelectMenuElement();
DCHECK(owner_select);

scoped_refptr<ComputedStyle> original_style =
OriginalStyleForLayoutObject(style_recalc_context);

LocalDOMWindow* window = GetDocument().domWindow();
if (!window)
return;
return original_style;

ComputedStyleBuilder style_builder(*original_style);

gfx::RectF anchor_rect_in_screen =
owner_select->GetBoundingClientRectNoLifecycleUpdate();
Expand All @@ -1841,41 +1847,44 @@ void HTMLElement::AdjustPopupPositionForSelectMenu(ComputedStyle& style) {
gfx::Rect avail_rect = gfx::Rect(0, 0, avail_width, avail_height);

// Remove any margins on the listbox part, so we can position it correctly.
style.SetMarginTop(Length::Fixed(0));
style.SetMarginLeft(Length::Fixed(0));
style.SetMarginRight(Length::Fixed(0));
style.SetMarginBottom(Length::Fixed(0));
ComputedStyle* style = style_builder.MutableInternalStyle();
style->SetMarginTop(Length::Fixed(0));
style->SetMarginLeft(Length::Fixed(0));
style->SetMarginRight(Length::Fixed(0));
style->SetMarginBottom(Length::Fixed(0));

// Position the listbox part where is more space available.
const float available_space_above =
anchor_rect_in_screen.y() - avail_rect.y();
const float available_space_below =
avail_rect.bottom() - anchor_rect_in_screen.bottom();
if (available_space_below < available_space_above) {
style.SetMaxHeight(Length::Fixed(available_space_above));
style.SetBottom(
style_builder.SetMaxHeight(Length::Fixed(available_space_above));
style_builder.SetBottom(
Length::Fixed(avail_rect.bottom() - anchor_rect_in_screen.y()));
style.SetTop(Length::Auto());
style_builder.SetTop(Length::Auto());
} else {
style.SetMaxHeight(Length::Fixed(available_space_below));
style.SetTop(Length::Fixed(anchor_rect_in_screen.bottom()));
style_builder.SetMaxHeight(Length::Fixed(available_space_below));
style_builder.SetTop(Length::Fixed(anchor_rect_in_screen.bottom()));
}

const float available_space_if_left_anchored =
avail_rect.right() - anchor_rect_in_screen.x();
const float available_space_if_right_anchored =
anchor_rect_in_screen.right() - avail_rect.x();
style.SetMinWidth(Length::Fixed(anchor_rect_in_screen.width()));
style_builder.SetMinWidth(Length::Fixed(anchor_rect_in_screen.width()));
if (available_space_if_left_anchored > anchor_rect_in_screen.width() ||
available_space_if_left_anchored > available_space_if_right_anchored) {
style.SetLeft(Length::Fixed(anchor_rect_in_screen.x()));
style.SetMaxWidth(Length::Fixed(available_space_if_left_anchored));
style_builder.SetLeft(Length::Fixed(anchor_rect_in_screen.x()));
style_builder.SetMaxWidth(Length::Fixed(available_space_if_left_anchored));
} else {
style.SetRight(
style_builder.SetRight(
Length::Fixed(avail_rect.right() - anchor_rect_in_screen.right()));
style.SetLeft(Length::Auto());
style.SetMaxWidth(Length::Fixed(available_space_if_right_anchored));
style_builder.SetLeft(Length::Auto());
style_builder.SetMaxWidth(Length::Fixed(available_space_if_right_anchored));
}

return style_builder.TakeStyle();
}

scoped_refptr<ComputedStyle> HTMLElement::CustomStyleForLayoutObject(
Expand All @@ -1888,10 +1897,7 @@ scoped_refptr<ComputedStyle> HTMLElement::CustomStyleForLayoutObject(
DCHECK(RuntimeEnabledFeatures::HTMLSelectMenuElementEnabled());
DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled(
GetDocument().GetExecutionContext()));
scoped_refptr<ComputedStyle> style =
OriginalStyleForLayoutObject(style_recalc_context);
AdjustPopupPositionForSelectMenu(*style);
return style;
return StyleForSelectMenuPopupstyle(style_recalc_context);
}
return Element::CustomStyleForLayoutObject(style_recalc_context);
}
Expand Down
3 changes: 2 additions & 1 deletion third_party/blink/renderer/core/html/html_element.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,8 @@ class CORE_EXPORT HTMLElement : public Element {
// anchored positioning scheme.
void SetNeedsRepositioningForSelectMenu(bool flag);
void SetOwnerSelectMenuElement(HTMLSelectMenuElement* element);
void AdjustPopupPositionForSelectMenu(ComputedStyle& style);
scoped_refptr<ComputedStyle> StyleForSelectMenuPopupstyle(
const StyleRecalcContext&);

bool DispatchFocusEvent(
Element* old_focused_element,
Expand Down
67 changes: 33 additions & 34 deletions third_party/blink/renderer/core/layout/ng/ng_relative_utils_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,59 +22,62 @@ const LayoutUnit kZero{0};
class NGRelativeUtilsTest : public testing::Test {
protected:
void SetUp() override {
style_ = ComputedStyle::CreateInitialStyleSingleton();
style_->SetPosition(EPosition::kRelative);
initial_style_ = ComputedStyle::CreateInitialStyleSingleton();
}

void SetTRBL(LayoutUnit top,
LayoutUnit right,
LayoutUnit bottom,
LayoutUnit left) {
style_->SetTop(top == kAuto ? Length::Auto() : Length::Fixed(top.ToInt()));
style_->SetRight(right == kAuto ? Length::Auto()
scoped_refptr<const ComputedStyle> CreateStyle(LayoutUnit top,
LayoutUnit right,
LayoutUnit bottom,
LayoutUnit left) {
ComputedStyleBuilder builder(*initial_style_);
builder.SetPosition(EPosition::kRelative);
builder.SetTop(top == kAuto ? Length::Auto() : Length::Fixed(top.ToInt()));
builder.SetRight(right == kAuto ? Length::Auto()
: Length::Fixed(right.ToInt()));
style_->SetBottom(bottom == kAuto ? Length::Auto()
builder.SetBottom(bottom == kAuto ? Length::Auto()
: Length::Fixed(bottom.ToInt()));
style_->SetLeft(left == kAuto ? Length::Auto()
builder.SetLeft(left == kAuto ? Length::Auto()
: Length::Fixed(left.ToInt()));
return builder.TakeStyle();
}

scoped_refptr<ComputedStyle> style_;
scoped_refptr<const ComputedStyle> initial_style_;
LogicalSize container_size_;
};

TEST_F(NGRelativeUtilsTest, HorizontalTB) {
LogicalOffset offset;

// Everything auto defaults to kZero,kZero
SetTRBL(kAuto, kAuto, kAuto, kAuto);
scoped_refptr<const ComputedStyle> style =
CreateStyle(kAuto, kAuto, kAuto, kAuto);
offset = ComputeRelativeOffset(
*style_, {WritingMode::kHorizontalTb, TextDirection::kLtr},
*style, {WritingMode::kHorizontalTb, TextDirection::kLtr},
container_size_);
EXPECT_EQ(offset.inline_offset, kZero);
EXPECT_EQ(offset.block_offset, kZero);

// Set all sides
SetTRBL(kTop, kRight, kBottom, kLeft);
style = CreateStyle(kTop, kRight, kBottom, kLeft);

// kLtr
offset = ComputeRelativeOffset(
*style_, {WritingMode::kHorizontalTb, TextDirection::kLtr},
*style, {WritingMode::kHorizontalTb, TextDirection::kLtr},
container_size_);
EXPECT_EQ(offset.inline_offset, kLeft);
EXPECT_EQ(offset.block_offset, kTop);

// kRtl
offset = ComputeRelativeOffset(
*style_, {WritingMode::kHorizontalTb, TextDirection::kRtl},
*style, {WritingMode::kHorizontalTb, TextDirection::kRtl},
container_size_);
EXPECT_EQ(offset.inline_offset, kRight);
EXPECT_EQ(offset.block_offset, kTop);

// Set only non-default sides
SetTRBL(kAuto, kRight, kBottom, kAuto);
style = CreateStyle(kAuto, kRight, kBottom, kAuto);
offset = ComputeRelativeOffset(
*style_, {WritingMode::kHorizontalTb, TextDirection::kLtr},
*style, {WritingMode::kHorizontalTb, TextDirection::kLtr},
container_size_);
EXPECT_EQ(offset.inline_offset, -kRight);
EXPECT_EQ(offset.block_offset, -kBottom);
Expand All @@ -84,27 +87,25 @@ TEST_F(NGRelativeUtilsTest, VerticalRightLeft) {
LogicalOffset offset;

// Set all sides
SetTRBL(kTop, kRight, kBottom, kLeft);
scoped_refptr<const ComputedStyle> style =
CreateStyle(kTop, kRight, kBottom, kLeft);

// kLtr
offset = ComputeRelativeOffset(
*style_, {WritingMode::kVerticalRl, TextDirection::kLtr},
container_size_);
*style, {WritingMode::kVerticalRl, TextDirection::kLtr}, container_size_);
EXPECT_EQ(offset.inline_offset, kTop);
EXPECT_EQ(offset.block_offset, kRight);

// kRtl
offset = ComputeRelativeOffset(
*style_, {WritingMode::kVerticalRl, TextDirection::kRtl},
container_size_);
*style, {WritingMode::kVerticalRl, TextDirection::kRtl}, container_size_);
EXPECT_EQ(offset.inline_offset, kBottom);
EXPECT_EQ(offset.block_offset, kRight);

// Set only non-default sides
SetTRBL(kAuto, kAuto, kBottom, kLeft);
style = CreateStyle(kAuto, kAuto, kBottom, kLeft);
offset = ComputeRelativeOffset(
*style_, {WritingMode::kVerticalRl, TextDirection::kLtr},
container_size_);
*style, {WritingMode::kVerticalRl, TextDirection::kLtr}, container_size_);
EXPECT_EQ(offset.inline_offset, -kBottom);
EXPECT_EQ(offset.block_offset, -kLeft);
}
Expand All @@ -113,27 +114,25 @@ TEST_F(NGRelativeUtilsTest, VerticalLeftRight) {
LogicalOffset offset;

// Set all sides
SetTRBL(kTop, kRight, kBottom, kLeft);
scoped_refptr<const ComputedStyle> style =
CreateStyle(kTop, kRight, kBottom, kLeft);

// kLtr
offset = ComputeRelativeOffset(
*style_, {WritingMode::kVerticalLr, TextDirection::kLtr},
container_size_);
*style, {WritingMode::kVerticalLr, TextDirection::kLtr}, container_size_);
EXPECT_EQ(offset.inline_offset, kTop);
EXPECT_EQ(offset.block_offset, kLeft);

// kRtl
offset = ComputeRelativeOffset(
*style_, {WritingMode::kVerticalLr, TextDirection::kRtl},
container_size_);
*style, {WritingMode::kVerticalLr, TextDirection::kRtl}, container_size_);
EXPECT_EQ(offset.inline_offset, kBottom);
EXPECT_EQ(offset.block_offset, kLeft);

// Set only non-default sides
SetTRBL(kAuto, kRight, kBottom, kAuto);
style = CreateStyle(kAuto, kRight, kBottom, kAuto);
offset = ComputeRelativeOffset(
*style_, {WritingMode::kVerticalLr, TextDirection::kLtr},
container_size_);
*style, {WritingMode::kVerticalLr, TextDirection::kLtr}, container_size_);
EXPECT_EQ(offset.inline_offset, -kBottom);
EXPECT_EQ(offset.block_offset, -kRight);
}
Expand Down

0 comments on commit 41e0f1e

Please sign in to comment.