From 1a49ba1e9e8c98f35b0b51990dcde66c445f3a9b Mon Sep 17 00:00:00 2001 From: Xiaocheng Hu <xiaochengh@chromium.org> Date: Fri, 29 Sep 2023 16:12:49 -0700 Subject: [PATCH] Reland "[anchor-position] Support margin properties in position fallback" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 426a7d8727ff4a2767fdd3cb22b5f091888aa39c. Reason for reland: ensured roundtrip for simple values - Reverted behavior (PS#1): check layout for all boxes - Old behavior (base): check layout for boxes with non-fixed style values - New behavior: check layout for boxes with either non-fixed style values or using position fallback style Original change's description: > Revert "[anchor-position] Support margin properties in position fallback" > > This reverts commit 502912cd04e83f9d1ce1d8df50fc9778b57207e0. > > Reason for revert: causing https://crbug.com/1485377 > > Original change's description: > > [anchor-position] Support margin properties in position fallback > > > > This patch makes margin properties (except margin-trim) allowed in a > > @position-fallback rule, so that different fallback positions can use > > different margins. > > > > There's also some changes to `getComputedStyle()` implementation to > > make it return the used margin values from fallback styles. To do so, > > we unconditionally obtain the result from a `LayoutBox` if the current > > object is a box. Since the result is available only after layout, this > > patch also changes all margin properties on boxes to be layout > > dependent, regardless of the computed margin values. > > > > Note: This causes some rounding differences, as the result from > > `LayoutBox`, which is the actual layout precision, has a fixed-point > > precision of 1/64 px. Therefore, some tests are rebaselined. > > > > Bug: 1475317 > > Change-Id: I4a3e242ceb04413db5348770b5cb666a54a1c9e0 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4819910 > > Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> > > Reviewed-by: Sami Kyöstilä <skyostil@chromium.org> > > Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org> > > Cr-Commit-Position: refs/heads/main@{#1190240} > > Bug: 1475317, 1485377 > Change-Id: Ieb200c75919ad7ad67745519791fcb9473c24e9b > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4881060 > Reviewed-by: Sami Kyöstilä <skyostil@chromium.org> > Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1199388} Bug: 1475317, 1485377 Change-Id: I95789a405a2e1bc71ec3b3819a8bb6c4ddd77ad1 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4885150 Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org> Auto-Submit: Xiaocheng Hu <xiaochengh@chromium.org> Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> Cr-Commit-Position: refs/heads/main@{#1203515} --- ...allback-position-allowed-declarations.html | 20 +++-- .../position-fallback-004.html | 74 +++++++++++++++++++ .../getComputedStyle-margins-roundtrip.html | 35 +++++++++ 3 files changed, 122 insertions(+), 7 deletions(-) create mode 100644 css/css-anchor-position/position-fallback-004.html create mode 100644 css/cssom/getComputedStyle-margins-roundtrip.html diff --git a/css/css-anchor-position/at-fallback-position-allowed-declarations.html b/css/css-anchor-position/at-fallback-position-allowed-declarations.html index 873fa13140047c0..25f6a14577f06bc 100644 --- a/css/css-anchor-position/at-fallback-position-allowed-declarations.html +++ b/css/css-anchor-position/at-fallback-position-allowed-declarations.html @@ -45,6 +45,19 @@ test_allowed_declaration('inset-inline'); test_allowed_declaration('inset'); +// Margin properties are allowed +test_allowed_declaration('margin-top'); +test_allowed_declaration('margin-bottom'); +test_allowed_declaration('margin-left'); +test_allowed_declaration('margin-right'); +test_allowed_declaration('margin-block-start'); +test_allowed_declaration('margin-block-end'); +test_allowed_declaration('margin-inline-start'); +test_allowed_declaration('margin-inline-end'); +test_allowed_declaration('margin-block'); +test_allowed_declaration('margin-inline'); +test_allowed_declaration('margin'); + // Sizing properties are allowed test_allowed_declaration('width'); test_allowed_declaration('height'); @@ -70,13 +83,6 @@ // Custom properties are disallowed test_disallowed_declaration('--custom'); -// Margin properties are disallowed -test_disallowed_declaration('margin-left'); -test_disallowed_declaration('margin-right'); -test_disallowed_declaration('margin-top'); -test_disallowed_declaration('margin-bottom'); -test_disallowed_declaration('margin'); - // Test some other disallowed properties test_disallowed_declaration('font-size'); test_disallowed_declaration('border-width'); diff --git a/css/css-anchor-position/position-fallback-004.html b/css/css-anchor-position/position-fallback-004.html new file mode 100644 index 000000000000000..e4dbd718664fca1 --- /dev/null +++ b/css/css-anchor-position/position-fallback-004.html @@ -0,0 +1,74 @@ +<!DOCTYPE html> +<title>Tests margin properties in position fallback</title> +<link rel="help" href="https://drafts.csswg.org/css-anchor-position-1/#accepted-try-properties"> +<link rel="author" href="mailto:xiaochengh@chromium.org"> +<script src="/resources/testharness.js"></script> +<script src="/resources/testharnessreport.js"></script> +<script src="/resources/check-layout-th.js"></script> +<script src="support/test-common.js"></script> + +<style> +body { + margin: 0; +} + +.cb { + width: 300px; + height: 150px; + position: relative; + background: lightgray; +} + +.anchor { + position: absolute; + width: 100px; + height: 100px; + top: 25px; + background: orange; + anchor-name: --a; +} + +.target { + position: absolute; + width: 100px; + height: 100px; + background: lime; + position-fallback: --fallbacks; +} + +@position-fallback --fallbacks { + @try { + top: anchor(--a top); + right: anchor(--a left); + margin-top: 10px; + margin-right: 10px; + } + + @try { + bottom: anchor(--a bottom); + left: anchor(--a right); + margin-bottom: 10px; + margin-left: 10px; + } +} +</style> + +<body onload="checkLayoutForAnchorPos('.target')"> + +<div class=cb> + <div class=anchor style="left: 110px"></div> + <!-- Chooses 1st @try block. --> + <div class=target data-offset-x=0 + data-expected-margin-left=0 data-expected-margin-right=10 + data-expected-margin-top=10 data-expected-margin-bottom=0></div> +</div> + +<div class=cb> + <div class=anchor style="right: 110px"></div> + <!-- Chooses 2nd @try block. --> + <div class=target data-offset-x=200 + data-expected-margin-left=10 data-expected-margin-right=0 + data-expected-margin-top=0 data-expected-margin-bottom=10></div> +</div> + +</body> diff --git a/css/cssom/getComputedStyle-margins-roundtrip.html b/css/cssom/getComputedStyle-margins-roundtrip.html new file mode 100644 index 000000000000000..8913be012d705ba --- /dev/null +++ b/css/cssom/getComputedStyle-margins-roundtrip.html @@ -0,0 +1,35 @@ +<!DOCTYPE html> +<title>Chromium bug: getComputedStyle() fixed-length inset values don't roundtrip</title> +<link rel="help" href="https://drafts.csswg.org/cssom/#resolved-value"> +<link rel="help" href="https://bugs.chromium.org/p/chromium/issues/detail?id=1482703"> +<link rel="help" href="mailto:xiaochengh@chromium.org"> +<script src="/resources/testharness.js"></script> +<script src="/resources/testharnessreport.js"></script> + +<div id="target1" style="margin-left: 20.7px"></div> +<script> +test(() => { + assert_equals(getComputedStyle(target1).marginLeft, '20.7px'); +}, 'Fixed-length margin-left property value should roundtrip'); +</script> + +<div id="target2" style="margin-right: 20.7px"></div> +<script> +test(() => { + assert_equals(getComputedStyle(target2).marginRight, '20.7px'); +}, 'Fixed-length margin-right property value should roundtrip'); +</script> + +<div id="target3" style="margin-top: 20.7px"></div> +<script> +test(() => { + assert_equals(getComputedStyle(target3).marginTop, '20.7px'); +}, 'Fixed-length margin-top property value should roundtrip'); +</script> + +<div id="target4" style="margin-bottom: 20.7px"></div> +<script> +test(() => { + assert_equals(getComputedStyle(target4).marginBottom, '20.7px'); +}, 'Fixed-length margin-bottom property value should roundtrip'); +</script>