Reland "Don't reset computed 'float' for flex and grid items"
This is a reland of 906abd41e4f6c786a1927f6edd69d6926e20317b
Original change :
https://chromium-review.googlesource.com/c/chromium/src/+/2157889
Revert of original change :
https://chromium-review.googlesource.com/c/chromium/src/+/2256772
-----
The original change was reverted due to a DCHECK being triggered. See
the following CRBug for more details:
https://bugs.chromium.org/p/chromium/issues/detail?id=1097595
The original change no longer adjusted the ComputedStyle for floats
based on whether or not it was a flex/grid item. This adjustment was
left for the LayoutBox to handle. However, there was code on the layout
side that depended on IsFloating() getting adjusted for the style, as
well. This resulted in the DCHEK mentioned above getting hit.
To fix this, I updated all cases that used the style's IsFloating()
function and either updated it to also take the style's
IsFlexOrGridItem() function into account or updated the code to use
LayoutObject's IsFloating() function instead.
-----
Original change's description:
> Don't reset computed 'float' for flex and grid items
>
> Floated flex and grid items have their 'float' incorrectly computed to
> "none". This change fixes this by adjusting IsFloating() for flex and
> grid items inside LayoutBox::UpdateFromStyle() instead of inside
> StyleAdjuster::AdjustStyleForDisplay().
>
> Beyond this, legend elements are not allowed to be rendered legends
> if they are floating. However, if legends are flex items, we
> adjust IsFloating() to be false. This causes legends to be
> used as rendered legends when they are flex items, even if they
> were styled as a float.
>
> This is fixed by checking ComputedStyle::IsFloating() instead
> of LayoutObject::IsFloating() when finding the rendered legend in
> order to get its non-adjusted floating value.
>
> Bug: 875235,350505
> Change-Id: Ia1e7f7c244cc0c443cd58be42854866884b7f7e7
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2157889
> Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
> Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
> Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
> Commit-Queue: Alison Maher <almaher@microsoft.com>
> Cr-Commit-Position: refs/heads/master@{#763988}
Bug: 875235, 350505
Change-Id: I915e045db7fda94d99cfdae33911d8b5f6df26de
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2261412
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Alison Maher <almaher@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#785456}
diff --git a/css/css-display/display-first-line-002-ref.html b/css/css-display/display-first-line-002-ref.html
new file mode 100644
index 0000000..e82f615
--- /dev/null
+++ b/css/css-display/display-first-line-002-ref.html
@@ -0,0 +1,6 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<div contenteditable></div>
+<p>The word "PASS" should be seen below. There should be no DCHECK
+ failure.</p>
+<p style="color: blue;">PASS</p>
diff --git a/css/css-display/display-first-line-002.html b/css/css-display/display-first-line-002.html
new file mode 100644
index 0000000..fef1d0b
--- /dev/null
+++ b/css/css-display/display-first-line-002.html
@@ -0,0 +1,28 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>Display flex first line with floated first letter</title>
+<link rel="help" href="https://crbug.com/1097595">
+<link rel="match" href="display-first-line-002-ref.html">
+<style id="styleElm">
+ #victim::first-line { display:flex; }
+ #victim::first-letter { float:right; }
+</style>
+
+<!-- The contenteditable DIV is just here to trigger legacy layout
+ fallback, and a DCHECK failure identical to the one in the bug
+ report. If we remove it, we'll get NG layout, and then it will
+ actually DCHECK-fail inside NG inline layout code instead. -->
+<div contenteditable></div>
+
+<p>The word "PASS" should be seen below. There should be no DCHECK
+ failure.</p>
+<div id="child" style="display:none;"></div>
+<div id="victim" style="width:fit-content;">
+ SPAS
+</div>
+<script>
+ document.body.offsetTop;
+ styleElm.appendChild(child);
+ document.body.offsetTop;
+ victim.style.color = "blue";
+</script>
diff --git a/css/css-display/display-with-float-dynamic.html b/css/css-display/display-with-float-dynamic.html
new file mode 100644
index 0000000..7cc2fef
--- /dev/null
+++ b/css/css-display/display-with-float-dynamic.html
@@ -0,0 +1,40 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>Computed float value of flex/grid items</title>
+<link rel="help" href="https://drafts.csswg.org/css-flexbox/#flex-containers">
+<link rel="help" href="https://drafts.csswg.org/css-grid-1/#grid-containers">
+<meta name="assert" content="computed float value of flex/grid items should be as specified">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+<div id="flex" style="display:flex;">
+ <div id="flex-item"></div>
+</div>
+<div id="grid" style="display:grid;">
+ <div id="grid-item">
+</div>
+<script>
+ function setFloatFor(id, float) {
+ document.getElementById(id).style.cssFloat = float;
+ }
+ function getFloatFor(id) {
+ return window.getComputedStyle(document.getElementById(id)).getPropertyValue("float");
+ }
+ function setDisplayBlock(id) {
+ document.getElementById(id).style.display = "block";
+ }
+ test(function() {
+ assert_equals(getFloatFor("flex-item"), "none");
+ assert_equals(getFloatFor("grid-item"), "none");
+
+ setFloatFor("flex-item", "left");
+ setFloatFor("grid-item", "right");
+ assert_equals(getFloatFor("flex-item"), "left");
+ assert_equals(getFloatFor("grid-item"), "right");
+
+ setDisplayBlock("grid");
+ setDisplayBlock("flex");
+ assert_equals(getFloatFor("flex-item"), "left");
+ assert_equals(getFloatFor("grid-item"), "right");
+ }, "computed style for float");
+</script>
diff --git a/css/css-display/display-with-float.html b/css/css-display/display-with-float.html
new file mode 100644
index 0000000..49f9479
--- /dev/null
+++ b/css/css-display/display-with-float.html
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>Computed float value of flex/grid items</title>
+<link rel="help" href="https://drafts.csswg.org/css-flexbox/#flex-containers">
+<link rel="help" href="https://drafts.csswg.org/css-grid-1/#grid-containers">
+<meta name="assert" content="computed float value of flex/grid items should be as specified">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+<div style="display:flex;">
+ <div id="flex-item" style="float:left;"></div>
+</div>
+<div style="display:grid;">
+ <div id="grid-item" style="float:right;"></div>
+</div>
+<script>
+ function getFloatFor(id) {
+ return window.getComputedStyle(document.getElementById(id)).getPropertyValue("float");
+ }
+ test(function() {
+ assert_equals(getFloatFor("flex-item"), "left");
+ assert_equals(getFloatFor("grid-item"), "right");
+ }, "computed style for float");
+</script>
diff --git a/html/rendering/non-replaced-elements/the-fieldset-and-legend-elements/flex-legend-float-abspos.html b/html/rendering/non-replaced-elements/the-fieldset-and-legend-elements/flex-legend-float-abspos.html
new file mode 100644
index 0000000..f6eead4
--- /dev/null
+++ b/html/rendering/non-replaced-elements/the-fieldset-and-legend-elements/flex-legend-float-abspos.html
@@ -0,0 +1,97 @@
+<!doctype html>
+<title>
+ legend and float and position: absolute/fixed when the display type of
+ the fieldset is flex.
+</title>
+<script src=/resources/testharness.js></script>
+<script src=/resources/testharnessreport.js></script>
+<style>
+ body { margin: 0; }
+ fieldset {
+ border: 10px solid;
+ display: flex;
+ margin: 0;
+ padding: 20px;
+ width: 300px;
+ }
+ legend { height: 10px; }
+ #legend1 { float: left; }
+ #legend2 { float: right; }
+ #legend3 { position: absolute; }
+ #legend4 { position: fixed; }
+</style>
+<fieldset id=fieldset>
+ <div>div</div>
+ <legend id=legend1>legend1</legend>
+ <legend id=legend2>legend2</legend>
+ <legend id=legend3>legend3</legend>
+ <legend id=legend4>legend4</legend>
+ <legend id=legend5>legend5</legend>
+</fieldset>
+<script>
+ const fieldset = document.getElementById('fieldset');
+ const legends = document.getElementsByTagName('legend');
+ const [legend1, legend2, legend3, legend4, legend5] = legends;
+ const expectedTop = 0;
+ const expectedLeft = 10 + 20;
+
+ function assert_rendered_legend(legend) {
+ assert_equals(legend.offsetTop, expectedTop, `${legend.id}.offsetTop`);
+ assert_equals(legend.offsetLeft, expectedLeft, `${legend.id}.offsetLeft`);
+ for (const other of legends) {
+ if (other === legend) {
+ continue;
+ }
+ if (other.offsetTop === expectedTop && other.offsetLeft === expectedLeft) {
+ assert_unreached(`${other.id} should not be the "rendered legend"`);
+ }
+ }
+ }
+
+ test(t => {
+ assert_rendered_legend(legend5);
+ }, 'no dynamic changes');
+
+ test(t => {
+ const legend = document.createElement('legend');
+ t.add_cleanup(() => {
+ legend.remove();
+ });
+ legend.id = 'script-inserted';
+ legend.textContent = 'script-inserted legend';
+ fieldset.insertBefore(legend, legend1);
+ assert_rendered_legend(legend);
+ legend.remove();
+ assert_rendered_legend(legend5);
+ }, 'inserting a new legend and removing it again');
+
+ test(t => {
+ t.add_cleanup(() => {
+ legend1.id = 'legend1';
+ legend2.id = 'legend2';
+ });
+ legend2.id = '';
+ assert_rendered_legend(legend2);
+ legend1.id = '';
+ assert_rendered_legend(legend1);
+ legend1.id = 'legend1';
+ assert_rendered_legend(legend2);
+ legend2.id = 'legend2';
+ assert_rendered_legend(legend5);
+ }, 'dynamic changes to float');
+
+ test(t => {
+ t.add_cleanup(() => {
+ legend3.id = 'legend3';
+ legend4.id = 'legend4';
+ });
+ legend4.id = '';
+ assert_rendered_legend(legend4);
+ legend3.id = '';
+ assert_rendered_legend(legend3);
+ legend3.id = 'legend3';
+ assert_rendered_legend(legend4);
+ legend4.id = 'legend4';
+ assert_rendered_legend(legend5);
+ }, 'dynamic changes to position');
+</script>
