Skip to content

GitLab

T
tangled
Commit cf88186a authored by Koji Ishii's avatar Koji Ishii Committed by Commit Bot
Browse files
Options

[LayoutNG] Fix `vertical-align: top` and `bottom` 

This patch fixes `vertical-align: top` and `bottom` when
they are applied to a box that has children with `text-top`
or `text-bottom`.

In LayoutNG, `vertical-align` is handled in 3 different
timings.
1. `top` and `bottom` are added to the pending list of the
   line box, or to the nearest ancestor box that has `top` or
   `bottom`.
2. `text-bop` and `text-bottom` are added to the pending list
   of the parent box.
3. Other values are computed when the box is closed.

When a box has both 1 and 2, this patch changes to compute 1
(`top` and `bottom`) after 2. Before this patch, both 1 and 2
are computed at the same time for each box. 3 test cases out
of 20 in this test fails without this patch.

Bug: 1001664
Change-Id: I0e7746447f5e401837c2c28e308171f0987eba35
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1792396Reviewed-by: default avatarEmil A Eklund <eae@chromium.org>
Reviewed-by: default avatarIan Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695194}
parent f5a731a3
Hide whitespace changes
Inline Side-by-side
Showing with 113 additions and 7 deletions
third_party/blink/renderer/core/layout/ng/inline/ng_inline_box_state.cc
View file @ cf88186a
...@@ -673,7 +673,7 @@ NGInlineLayoutStateStack::ApplyBaselineShift( ...@@ -673,7 +673,7 @@ NGInlineLayoutStateStack::ApplyBaselineShift(
// |pending_descendants|. // |pending_descendants|.
LayoutUnit baseline_shift; LayoutUnit baseline_shift;
if (!box->pending_descendants.IsEmpty()) { if (!box->pending_descendants.IsEmpty()) {
NGLineHeightMetrics max = MetricsForTopAndBottomAlign(*box, *line_box); bool has_top_or_bottom = false;
for (NGPendingPositions& child : box->pending_descendants) { for (NGPendingPositions& child : box->pending_descendants) {
// In quirks mode, metrics is empty if no content. // In quirks mode, metrics is empty if no content.
if (child.metrics.IsEmpty()) if (child.metrics.IsEmpty())
...@@ -682,9 +682,6 @@ NGInlineLayoutStateStack::ApplyBaselineShift( ...@@ -682,9 +682,6 @@ NGInlineLayoutStateStack::ApplyBaselineShift(
case EVerticalAlign::kTextTop: case EVerticalAlign::kTextTop:
baseline_shift = child.metrics.ascent + box->TextTop(baseline_type); baseline_shift = child.metrics.ascent + box->TextTop(baseline_type);
break; break;
case EVerticalAlign::kTop:
baseline_shift = child.metrics.ascent - max.ascent;
break;
case EVerticalAlign::kTextBottom: case EVerticalAlign::kTextBottom:
if (const SimpleFontData* font_data = if (const SimpleFontData* font_data =
box->style->GetFont().PrimaryFont()) { box->style->GetFont().PrimaryFont()) {
...@@ -694,10 +691,11 @@ NGInlineLayoutStateStack::ApplyBaselineShift( ...@@ -694,10 +691,11 @@ NGInlineLayoutStateStack::ApplyBaselineShift(
break; break;
} }
NOTREACHED(); NOTREACHED();
FALLTHROUGH;
case EVerticalAlign::kBottom:
baseline_shift = max.descent - child.metrics.descent;
break; break;
case EVerticalAlign::kTop:
case EVerticalAlign::kBottom:
has_top_or_bottom = true;
continue;
default: default:
NOTREACHED(); NOTREACHED();
continue; continue;
...@@ -707,6 +705,32 @@ NGInlineLayoutStateStack::ApplyBaselineShift( ...@@ -707,6 +705,32 @@ NGInlineLayoutStateStack::ApplyBaselineShift(
line_box->MoveInBlockDirection(baseline_shift, child.fragment_start, line_box->MoveInBlockDirection(baseline_shift, child.fragment_start,
child.fragment_end); child.fragment_end);
} }
// `top` and `bottom` need to be applied after all other values are applied,
// because they align to the maximum metrics, but the maximum metrics may
// depend on other pending descendants for this box.
if (has_top_or_bottom) {
NGLineHeightMetrics max = MetricsForTopAndBottomAlign(*box, *line_box);
for (NGPendingPositions& child : box->pending_descendants) {
switch (child.vertical_align) {
case EVerticalAlign::kTop:
baseline_shift = child.metrics.ascent - max.ascent;
break;
case EVerticalAlign::kBottom:
baseline_shift = max.descent - child.metrics.descent;
break;
case EVerticalAlign::kTextTop:
case EVerticalAlign::kTextBottom:
continue;
default:
NOTREACHED();
continue;
}
child.metrics.Move(baseline_shift);
box->metrics.Unite(child.metrics);
line_box->MoveInBlockDirection(baseline_shift, child.fragment_start,
child.fragment_end);
}
}
box->pending_descendants.clear(); box->pending_descendants.clear();
} }
......
third_party/blink/web_tests/external/wpt/css/CSS2/linebox/vertical-align-top-bottom-001.html 0 → 100644
View file @ cf88186a
<!DOCTYPE html>
<link rel="help" href="https://drafts.csswg.org/css2/visudet.html#propdef-vertical-align" />
<link rel="author" href="mailto:kojii@chromium.org">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<style>
section.test {
display: inline-block;
font-size: 20px;
line-height: 1.5;
font-family: Arial;
font-family: Ahem;
}
section.test > div {
background: blue;
margin-bottom: 1em;
}
.filler {
display: inline-block;
background: cyan;
height: 3em;
width: 1em;
}
.target {
display: inline-block;
background: orange;
width: 1em;
height: 1em;
}
div.top, section.top .target { vertical-align: top; }
div.text-top, section.text-top .target { vertical-align: text-top; }
div.text-bottom, section.text-bottom .target { vertical-align: text-bottom; }
div.bottom, section.bottom .target { vertical-align: bottom; }
.test .fail {
outline: red solid 5px;
}
</style>
<body>
<section class="test top">
<div><div class="filler"></div><div class="target" data-y="0"></div></div>
<div><div class="filler top"></div><div class="target" data-y="0"></div></div>
<div><div class="filler text-top"></div><div class="target" data-y="0"></div></div>
<div><div class="filler bottom"></div><div class="target" data-y="0"></div></div>
<div><div class="filler text-bottom"></div><div class="target" data-y="0"></div></div>
</section>
<section class="test text-top">
<div><div class="filler"></div><div class="target" data-y="44"></div></div>
<div><div class="filler top"></div><div class="target" data-y="5"></div></div>
<div><div class="filler text-top"></div><div class="target" data-y="5"></div></div>
<div><div class="filler bottom"></div><div class="target" data-y="35"></div></div>
<div><div class="filler text-bottom"></div><div class="target" data-y="40"></div></div>
</section>
<section class="test text-bottom">
<div><div class="filler"></div><div class="target" data-y="44"></div></div>
<div><div class="filler top"></div><div class="target" data-y="5"></div></div>
<div><div class="filler text-top"></div><div class="target" data-y="5"></div></div>
<div><div class="filler bottom"></div><div class="target" data-y="35"></div></div>
<div><div class="filler text-bottom"></div><div class="target" data-y="40"></div></div>
</section>
<section class="test bottom">
<div><div class="filler"></div><div class="target" data-y="49"></div></div>
<div><div class="filler top"></div><div class="target" data-y="40"></div></div>
<div><div class="filler text-top"></div><div class="target" data-y="45"></div></div>
<div><div class="filler bottom"></div><div class="target" data-y="40"></div></div>
<div><div class="filler text-bottom"></div><div class="target" data-y="45"></div></div>
</section>
<script>
for (let target of document.getElementsByClassName('target')) {
let container = target.parentElement;
let filler = container.firstElementChild;
let section = container.parentElement;
let pass = false;
test(() => {
let y = target.offsetTop - container.offsetTop;
assert_approx_equals(y, target.dataset.y, 0);
pass = true;
}, `${section.className.substr(5)}+${filler.className.substr(7)}`);
if (!pass)
container.classList.add('fail');
}
</script>
</body>
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment