[LFC][IFC] Fix various vertical alignment issues.
https://bugs.webkit.org/show_bug.cgi?id=203041
Reviewed by Antti Koivisto.
This patch fixes both regular inline box and inline-block baseline aligment. It also addresses a few related vertical positioning issues.
* layout/inlineformatting/InlineLine.cpp:
(WebCore::Layout::Line::alignContentVertically):
(WebCore::Layout::Line::appendInlineContainerStart):
(WebCore::Layout::Line::appendInlineContainerEnd):
(WebCore::Layout::Line::appendTextContent):
(WebCore::Layout::Line::appendNonReplacedInlineBox):
(WebCore::Layout::Line::appendHardLineBreak):
(WebCore::Layout::Line::adjustBaselineAndLineHeight):
(WebCore::Layout::Line::inlineItemContentHeight const):
* layout/inlineformatting/InlineLine.h:
* layout/inlineformatting/InlineLineBox.h:
(WebCore::Layout::LineBox::resetDescent):
(WebCore::Layout::LineBox::setLogicalHeightIfGreater):
(WebCore::Layout::LineBox::setBaselineOffsetIfGreater):
(WebCore::Layout::LineBox::setAscentIfGreater):
(WebCore::Layout::LineBox::setDescentIfGreater):
(WebCore::Layout::LineBox::resetBaseline):
(WebCore::Layout::LineBox::setBaseline): Deleted.
(WebCore::Layout::LineBox::baseline): Deleted.
(WebCore::Layout::LineBox::setBaselineOffset): Deleted.
(WebCore::Layout::LineBox::Baseline::setAscentIfGreater): Deleted.
(WebCore::Layout::LineBox::Baseline::setDescentIfGreater): Deleted.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@251200 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 1be2437..f47c526 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,35 @@
+2019-10-16 Zalan Bujtas <zalan@apple.com>
+
+ [LFC][IFC] Fix various vertical alignment issues.
+ https://bugs.webkit.org/show_bug.cgi?id=203041
+
+ Reviewed by Antti Koivisto.
+
+ This patch fixes both regular inline box and inline-block baseline aligment. It also addresses a few related vertical positioning issues.
+
+ * layout/inlineformatting/InlineLine.cpp:
+ (WebCore::Layout::Line::alignContentVertically):
+ (WebCore::Layout::Line::appendInlineContainerStart):
+ (WebCore::Layout::Line::appendInlineContainerEnd):
+ (WebCore::Layout::Line::appendTextContent):
+ (WebCore::Layout::Line::appendNonReplacedInlineBox):
+ (WebCore::Layout::Line::appendHardLineBreak):
+ (WebCore::Layout::Line::adjustBaselineAndLineHeight):
+ (WebCore::Layout::Line::inlineItemContentHeight const):
+ * layout/inlineformatting/InlineLine.h:
+ * layout/inlineformatting/InlineLineBox.h:
+ (WebCore::Layout::LineBox::resetDescent):
+ (WebCore::Layout::LineBox::setLogicalHeightIfGreater):
+ (WebCore::Layout::LineBox::setBaselineOffsetIfGreater):
+ (WebCore::Layout::LineBox::setAscentIfGreater):
+ (WebCore::Layout::LineBox::setDescentIfGreater):
+ (WebCore::Layout::LineBox::resetBaseline):
+ (WebCore::Layout::LineBox::setBaseline): Deleted.
+ (WebCore::Layout::LineBox::baseline): Deleted.
+ (WebCore::Layout::LineBox::setBaselineOffset): Deleted.
+ (WebCore::Layout::LineBox::Baseline::setAscentIfGreater): Deleted.
+ (WebCore::Layout::LineBox::Baseline::setDescentIfGreater): Deleted.
+
2019-10-16 Chris Dumez <cdumez@apple.com>
Drop unused WKPageSetResourceCachingDisabled() SPI
diff --git a/Source/WebCore/layout/inlineformatting/InlineLine.cpp b/Source/WebCore/layout/inlineformatting/InlineLine.cpp
index cfd9fe4..b819ff7 100644
--- a/Source/WebCore/layout/inlineformatting/InlineLine.cpp
+++ b/Source/WebCore/layout/inlineformatting/InlineLine.cpp
@@ -69,7 +69,7 @@
auto initialBaselineOffset = initialConstraints.heightAndBaseline ? initialConstraints.heightAndBaseline->baselineOffset : LayoutUnit();
auto lineRect = Display::Rect { initialConstraints.logicalTopLeft, { }, initialLineHeight };
auto baseline = LineBox::Baseline { initialBaselineOffset, initialLineHeight - initialBaselineOffset };
- m_lineBox = LineBox { lineRect, baseline, { } };
+ m_lineBox = LineBox { lineRect, baseline, initialBaselineOffset };
}
static bool isInlineContainerConsideredEmpty(const FormattingContext& formattingContext, const Box& layoutBox)
@@ -144,15 +144,14 @@
ASSERT(!m_skipAlignment);
if (isVisuallyEmpty()) {
- m_lineBox.baseline().reset();
- m_lineBox.setBaselineOffset({ });
+ m_lineBox.resetBaseline();
m_lineBox.setLogicalHeight({ });
}
// Remove descent when all content is baseline aligned but none of them have descent.
if (formattingContext().quirks().lineDescentNeedsCollapsing(m_runList)) {
m_lineBox.shrinkVertically(m_lineBox.baseline().descent());
- m_lineBox.baseline().setDescent({ });
+ m_lineBox.resetDescent();
}
auto& layoutState = this->layoutState();
@@ -174,8 +173,24 @@
auto& formattingState = downcast<InlineFormattingState>(layoutState.establishedFormattingState(downcast<Container>(layoutBox)));
// Spec makes us generate at least one line -even if it is empty.
ASSERT(!formattingState.lineBoxes().isEmpty());
- auto inlineBlockBaseline = formattingState.lineBoxes().last().baseline();
- logicalTop = baselineOffset() - inlineBlockBaseline.ascent();
+ auto inlineBlockBaselineOffset = formattingState.lineBoxes().last().baselineOffset();
+ // The inline-block's baseline offset is relative to its content box. Let's convert it relative to the margin box.
+ // inline-block
+ // \
+ // _______________ <- margin box
+ // |
+ // | ____________ <- border box
+ // | |
+ // | | _________ <- content box
+ // | | | ^
+ // | | | | <- baseline offset
+ // | | | |
+ // text | | | v text
+ // -----|-|-|---------- <- baseline
+ //
+ auto& boxGeometry = formattingContext.geometryForBox(layoutBox);
+ auto baselineOffsetFromMarginBox = boxGeometry.marginBefore() + boxGeometry.borderTop() + boxGeometry.paddingTop().valueOr(0) + inlineBlockBaselineOffset;
+ logicalTop = baselineOffset() - baselineOffsetFromMarginBox;
} else
logicalTop = baselineOffset() - run->logicalRect().height();
break;
@@ -294,9 +309,8 @@
auto logicalRect = Display::Rect { 0, contentLogicalWidth(), logicalWidth, 0 };
if (!m_skipAlignment) {
- auto logicalHeight = inlineItemContentHeight(inlineItem);
- adjustBaselineAndLineHeight(inlineItem, logicalHeight);
- logicalRect.setHeight(logicalHeight);
+ adjustBaselineAndLineHeight(inlineItem);
+ logicalRect.setHeight(inlineItemContentHeight(inlineItem));
}
appendNonBreakableSpace(inlineItem, logicalRect);
}
@@ -304,7 +318,7 @@
void Line::appendInlineContainerEnd(const InlineItem& inlineItem, LayoutUnit logicalWidth)
{
// This is really just a placeholder to mark the end of the inline level container </span>.
- auto logicalRect = Display::Rect { 0, contentLogicalRight(), logicalWidth, inlineItemContentHeight(inlineItem) };
+ auto logicalRect = Display::Rect { 0, contentLogicalRight(), logicalWidth, m_skipAlignment ? LayoutUnit() : inlineItemContentHeight(inlineItem) };
appendNonBreakableSpace(inlineItem, logicalRect);
}
@@ -341,9 +355,8 @@
logicalRect.setLeft(contentLogicalWidth());
logicalRect.setWidth(logicalWidth);
if (!m_skipAlignment) {
- auto runHeight = inlineItemContentHeight(inlineItem);
- logicalRect.setHeight(runHeight);
- adjustBaselineAndLineHeight(inlineItem, runHeight);
+ adjustBaselineAndLineHeight(inlineItem);
+ logicalRect.setHeight(inlineItemContentHeight(inlineItem));
}
auto lineItem = makeUnique<Run>(inlineItem, Display::Run { logicalRect, Display::Run::TextContext { inlineItem.start(), inlineItem.isCollapsed() ? 1 : inlineItem.length() } });
@@ -368,8 +381,9 @@
logicalRect.setLeft(contentLogicalWidth() + horizontalMargin.start);
logicalRect.setWidth(logicalWidth);
if (!m_skipAlignment) {
- adjustBaselineAndLineHeight(inlineItem, boxGeometry.marginBoxHeight());
- logicalRect.setHeight(inlineItemContentHeight(inlineItem));
+ adjustBaselineAndLineHeight(inlineItem);
+ auto runHeight = formattingContext().geometryForBox(inlineItem.layoutBox()).marginBoxHeight();
+ logicalRect.setHeight(runHeight);
}
m_runList.append(makeUnique<Run>(inlineItem, Display::Run { logicalRect }));
@@ -389,13 +403,13 @@
logicalRect.setLeft(contentLogicalWidth());
logicalRect.setWidth({ });
if (!m_skipAlignment) {
- adjustBaselineAndLineHeight(inlineItem, { });
+ adjustBaselineAndLineHeight(inlineItem);
logicalRect.setHeight(logicalHeight());
}
m_runList.append(makeUnique<Run>(inlineItem, Display::Run { logicalRect }));
}
-void Line::adjustBaselineAndLineHeight(const InlineItem& inlineItem, LayoutUnit runHeight)
+void Line::adjustBaselineAndLineHeight(const InlineItem& inlineItem)
{
ASSERT(!inlineItem.isContainerEnd());
auto& layoutBox = inlineItem.layoutBox();
@@ -406,18 +420,16 @@
// Inline containers stretch the line by their font size.
// Vertical margins, paddings and borders don't contribute to the line height.
auto& fontMetrics = style.fontMetrics();
- LayoutUnit contentHeight;
if (style.verticalAlign() == VerticalAlign::Baseline) {
auto halfLeading = halfLeadingMetrics(fontMetrics, style.computedLineHeight());
// Both halfleading ascent and descent could be negative (tall font vs. small line-height value)
if (halfLeading.descent() > 0)
- baseline.setDescentIfGreater(halfLeading.descent());
+ m_lineBox.setDescentIfGreater(halfLeading.descent());
if (halfLeading.ascent() > 0)
- baseline.setAscentIfGreater(halfLeading.ascent());
- contentHeight = baseline.height();
+ m_lineBox.setAscentIfGreater(halfLeading.ascent());
+ m_lineBox.setLogicalHeightIfGreater(baseline.height());
} else
- contentHeight = fontMetrics.height();
- m_lineBox.setLogicalHeightIfGreater(contentHeight, LineBox::AdjustBaseline::Yes);
+ m_lineBox.setLogicalHeightIfGreater(fontMetrics.height());
return;
}
@@ -426,37 +438,53 @@
// through the inline container (start) -see above. Normally the text content itself does not stretch the line.
if (!m_initialStrut)
return;
- baseline.setAscentIfGreater(m_initialStrut->ascent());
- baseline.setDescentIfGreater(m_initialStrut->descent());
- m_lineBox.setLogicalHeightIfGreater(baseline.height(), LineBox::AdjustBaseline::Yes);
+ m_lineBox.setAscentIfGreater(m_initialStrut->ascent());
+ m_lineBox.setDescentIfGreater(m_initialStrut->descent());
+ m_lineBox.setLogicalHeightIfGreater(baseline.height());
m_initialStrut = { };
return;
}
if (inlineItem.isBox()) {
+ auto& boxGeometry = formattingContext().geometryForBox(layoutBox);
+ auto marginBoxHeight = boxGeometry.marginBoxHeight();
+
switch (style.verticalAlign()) {
case VerticalAlign::Baseline: {
- auto newBaselineCandidate = LineBox::Baseline { runHeight, 0 };
if (layoutBox.isInlineBlockBox() && layoutBox.establishesInlineFormattingContext()) {
// Inline-blocks with inline content always have baselines.
auto& formattingState = downcast<InlineFormattingState>(layoutState().establishedFormattingState(downcast<Container>(layoutBox)));
// Spec makes us generate at least one line -even if it is empty.
ASSERT(!formattingState.lineBoxes().isEmpty());
- newBaselineCandidate = formattingState.lineBoxes().last().baseline();
+ auto& lastLineBox = formattingState.lineBoxes().last();
+ auto inlineBlockBaseline = lastLineBox.baseline();
+ auto beforeHeight = boxGeometry.marginBefore() + boxGeometry.borderTop() + boxGeometry.paddingTop().valueOr(0);
+
+ m_lineBox.setAscentIfGreater(inlineBlockBaseline.ascent());
+ m_lineBox.setDescentIfGreater(inlineBlockBaseline.descent());
+ m_lineBox.setBaselineOffsetIfGreater(beforeHeight + lastLineBox.baselineOffset());
+ m_lineBox.setLogicalHeightIfGreater(marginBoxHeight);
+ } else {
+ // Non inline-block boxes sit on the baseline (including their bottom margin).
+ m_lineBox.setAscentIfGreater(marginBoxHeight);
+ // Ignore negative descent (yes, negative descent is a thing).
+ m_lineBox.setLogicalHeightIfGreater(marginBoxHeight + std::max(LayoutUnit(), baseline.descent()));
}
- baseline.setAscentIfGreater(newBaselineCandidate.ascent());
- baseline.setDescentIfGreater(newBaselineCandidate.descent());
- m_lineBox.setLogicalHeightIfGreater(std::max(runHeight, baseline.height()), LineBox::AdjustBaseline::Yes);
break;
}
case VerticalAlign::Top:
- // Top align content never changes the baseline offset, it only pushes the bottom of the line further down.
- m_lineBox.setLogicalHeightIfGreater(runHeight, LineBox::AdjustBaseline::No);
+ // Top align content never changes the baseline, it only pushes the bottom of the line further down.
+ m_lineBox.setLogicalHeightIfGreater(marginBoxHeight);
break;
- case VerticalAlign::Bottom:
+ case VerticalAlign::Bottom: {
// Bottom aligned, tall content pushes the baseline further down from the line top.
- m_lineBox.setLogicalHeightIfGreater(runHeight, LineBox::AdjustBaseline::Yes);
+ auto lineLogicalHeight = m_lineBox.logicalHeight();
+ if (marginBoxHeight > lineLogicalHeight) {
+ m_lineBox.setLogicalHeightIfGreater(marginBoxHeight);
+ m_lineBox.setBaselineOffsetIfGreater(m_lineBox.baselineOffset() + (marginBoxHeight - lineLogicalHeight));
+ }
break;
+ }
default:
ASSERT_NOT_IMPLEMENTED_YET();
break;
@@ -476,17 +504,14 @@
auto& layoutBox = inlineItem.layoutBox();
auto& boxGeometry = formattingContext().geometryForBox(layoutBox);
- if (layoutBox.isFloatingPositioned())
- return boxGeometry.borderBoxHeight();
-
- if (layoutBox.replaced())
- return boxGeometry.borderBoxHeight();
+ if (layoutBox.replaced() || layoutBox.isFloatingPositioned())
+ return boxGeometry.contentBoxHeight();
if (inlineItem.isContainerStart() || inlineItem.isContainerEnd())
- return fontMetrics.height() + boxGeometry.verticalBorder() + boxGeometry.verticalPadding().valueOr(0);
+ return fontMetrics.height();
- // Non-replaced inline box (e.g. inline-block)
- return boxGeometry.borderBoxHeight();
+ // Non-replaced inline box (e.g. inline-block). It looks a bit misleading but their margin box is considered the content height here.
+ return boxGeometry.marginBoxHeight();
}
LineBox::Baseline Line::halfLeadingMetrics(const FontMetrics& fontMetrics, LayoutUnit lineLogicalHeight)
diff --git a/Source/WebCore/layout/inlineformatting/InlineLine.h b/Source/WebCore/layout/inlineformatting/InlineLine.h
index f6f33f2..03b88a3 100644
--- a/Source/WebCore/layout/inlineformatting/InlineLine.h
+++ b/Source/WebCore/layout/inlineformatting/InlineLine.h
@@ -129,7 +129,7 @@
void alignContentHorizontally();
void alignContentVertically();
- void adjustBaselineAndLineHeight(const InlineItem&, LayoutUnit runHeight);
+ void adjustBaselineAndLineHeight(const InlineItem&);
LayoutUnit inlineItemContentHeight(const InlineItem&) const;
bool isVisuallyEmpty() const;
diff --git a/Source/WebCore/layout/inlineformatting/InlineLineBox.h b/Source/WebCore/layout/inlineformatting/InlineLineBox.h
index 53e59f3..d1bd051 100644
--- a/Source/WebCore/layout/inlineformatting/InlineLineBox.h
+++ b/Source/WebCore/layout/inlineformatting/InlineLineBox.h
@@ -40,8 +40,6 @@
void setAscent(LayoutUnit);
void setDescent(LayoutUnit);
- void setAscentIfGreater(LayoutUnit);
- void setDescentIfGreater(LayoutUnit);
void reset();
@@ -71,9 +69,7 @@
LayoutUnit logicalWidth() const { return m_rect.width(); }
LayoutUnit logicalHeight() const { return m_rect.height(); }
- void setBaseline(Baseline);
const Baseline& baseline() const;
- Baseline& baseline();
// Baseline offset from line logical top. Note that offset does not necessarily equal to ascent.
//
// ------------------- line logical top ------------------- (top align)
@@ -90,13 +86,17 @@
// v
// ------------------- line logical bottom -------------------
LayoutUnit baselineOffset() const;
- void setBaselineOffset(LayoutUnit);
+ void setBaselineOffsetIfGreater(LayoutUnit);
+ void setAscentIfGreater(LayoutUnit);
+ void setDescentIfGreater(LayoutUnit);
+
+ void resetBaseline();
+ void resetDescent() { m_baseline.setDescent({ }); }
void setLogicalTopLeft(LayoutPoint logicalTopLeft) { m_rect.setTopLeft(logicalTopLeft); }
void setLogicalHeight(LayoutUnit logicalHeight) { m_rect.setHeight(logicalHeight); }
- enum class AdjustBaseline { No, Yes };
- void setLogicalHeightIfGreater(LayoutUnit logicalHeight, AdjustBaseline);
+ void setLogicalHeightIfGreater(LayoutUnit);
void setLogicalWidth(LayoutUnit logicalWidth) { m_rect.setWidth(logicalWidth); }
void moveHorizontally(LayoutUnit delta) { m_rect.moveHorizontally(delta); }
@@ -128,42 +128,40 @@
#endif
}
-inline void LineBox::setLogicalHeightIfGreater(LayoutUnit logicalHeight, AdjustBaseline adjustBaseline)
+inline void LineBox::setLogicalHeightIfGreater(LayoutUnit logicalHeight)
{
- auto diff = logicalHeight - m_rect.height();
- if (diff <= 0)
+ if (logicalHeight <= m_rect.height())
return;
- if (adjustBaseline == AdjustBaseline::Yes)
- setBaselineOffset(baselineOffset() + diff);
m_rect.setHeight(logicalHeight);
}
-inline void LineBox::setBaseline(Baseline baseline)
-{
-#if !ASSERT_DISABLED
- m_hasValidBaseline = true;
-#endif
- m_baseline = baseline;
-}
-
inline const LineBox::Baseline& LineBox::baseline() const
{
ASSERT(m_hasValidBaseline);
return m_baseline;
}
-inline LineBox::Baseline& LineBox::baseline()
-{
- ASSERT(m_hasValidBaseline);
- return m_baseline;
-}
-
-inline void LineBox::setBaselineOffset(LayoutUnit baselineOffset)
+inline void LineBox::setBaselineOffsetIfGreater(LayoutUnit baselineOffset)
{
#if !ASSERT_DISABLED
m_hasValidBaselineOffset = true;
#endif
- m_baselineOffset = baselineOffset;
+ m_baselineOffset = std::max(baselineOffset, m_baselineOffset);
+}
+
+inline void LineBox::setAscentIfGreater(LayoutUnit ascent)
+{
+ if (ascent < m_baseline.ascent())
+ return;
+ setBaselineOffsetIfGreater(ascent);
+ m_baseline.setAscent(ascent);
+}
+
+inline void LineBox::setDescentIfGreater(LayoutUnit descent)
+{
+ if (descent < m_baseline.descent())
+ return;
+ m_baseline.setDescent(descent);
}
inline LayoutUnit LineBox::baselineOffset() const
@@ -172,6 +170,15 @@
return m_baselineOffset;
}
+inline void LineBox::resetBaseline()
+{
+#if !ASSERT_DISABLED
+ m_hasValidBaselineOffset = true;
+#endif
+ m_baselineOffset = { };
+ m_baseline.reset();
+}
+
inline LineBox::Baseline::Baseline(LayoutUnit ascent, LayoutUnit descent)
: m_ascent(ascent)
, m_descent(descent)
@@ -198,22 +205,6 @@
m_descent = descent;
}
-inline void LineBox::Baseline::setAscentIfGreater(LayoutUnit ascent)
-{
-#if !ASSERT_DISABLED
- m_hasValidAscent = true;
-#endif
- m_ascent = std::max(ascent, m_ascent);
-}
-
-inline void LineBox::Baseline::setDescentIfGreater(LayoutUnit descent)
-{
-#if !ASSERT_DISABLED
- m_hasValidDescent = true;
-#endif
- m_descent = std::max(descent, m_descent);
-}
-
inline void LineBox::Baseline::reset()
{
#if !ASSERT_DISABLED
