Commit 6dc8794a authored by Aleks Totic's avatar Aleks Totic Committed by Commit Bot

[LayoutNG] Bugfix: ScrollableOverflow for PhysicalLineBoxFragment

Part 1: PhysicalLineFragment ScrollableOverflow

ScrollableOverflow was not taking relative position into account.
PhyiscalLineFragment used to recompute overflow when created.
This can't be done if relative position is needed, because
resolving % relative position needs size of containing block.

New code computes ScrollbleOverflow dynamically for PhysicalLineFragment.

Part 2: Make RelativeUtils API physical.

It is a better fit for PhysicalFragments.

Part 3: NGMixin::AddScrollableOverflow should also add overflow from
oof children.

Another tricky bug: If ContainingBlock() is inline, Legacy positions
OOF block wrt inline block, while NG positions OOF block wrt inline's
block container. Because OOF blocks affect scrollable overflow,
NG must be responsible for making sure OOF block is included in
container's overflow.

This patch makes 3 tests pass, and 1 new test fail:

compositing/culling/tile-occlusion-boundaries.html

The failure is caused by NG not using transfroms for
scrollable overflow computation. To fix this, we must make fragments
tranform-aware. Filed a bug to do so.

Bug: 728378
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: Ib511a95cc71648e806d0cc1376f1f6303152ecee
Reviewed-on: https://chromium-review.googlesource.com/1113030
Commit-Queue: Aleks Totic <atotic@chromium.org>
Reviewed-by: default avatarKoji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570171}
parent 96722ce9
......@@ -73,6 +73,7 @@ crbug.com/591099 fast/dom/inner-text-first-letter.html [ Pass ]
crbug.com/591099 animations/cross-fade-list-style-image.html [ Failure ]
crbug.com/591099 animations/rotate-transform-equivalent.html [ Failure ]
crbug.com/714962 compositing/background-color/view-blending-base-background.html [ Failure ]
crbug.com/728378 compositing/culling/tile-occlusion-boundaries.html [ Failure ]
crbug.com/591099 compositing/geometry/bounds-ignores-hidden.html [ Failure ]
crbug.com/591099 compositing/iframes/floating-self-painting-frame.html [ Failure ]
crbug.com/591099 compositing/self-painting-layers.html [ Failure ]
......@@ -354,9 +355,7 @@ crbug.com/810370 fast/block/float/overhanging-float-remove-from-fixed-position-b
crbug.com/591099 fast/block/float/overlapping-floats-paint-hittest-order-1.html [ Failure ]
crbug.com/591099 fast/block/line-layout/floats-do-not-fit-on-line.html [ Failure ]
crbug.com/591099 fast/block/over-constrained-auto-margin.html [ Failure ]
crbug.com/591099 fast/block/positioning/complex-positioned-movement-inline-ancestor.html [ Failure ]
crbug.com/591099 fast/block/positioning/positioned-child-inside-relative-positioned-anonymous-block.html [ Failure ]
crbug.com/591099 fast/block/positioning/relative-overflow-replaced.html [ Failure ]
crbug.com/591099 fast/borders/bidi-002.html [ Failure ]
crbug.com/591099 fast/borders/bidi-012.html [ Failure ]
crbug.com/591099 fast/borders/border-image-border-radius.html [ Failure ]
......@@ -422,7 +421,6 @@ crbug.com/591099 fast/css/import_with_baseurl.html [ Failure ]
crbug.com/591099 fast/css/negative-text-indent-in-inline-block.html [ Failure ]
crbug.com/591099 fast/css/opacity-float.html [ Pass ]
crbug.com/591099 fast/css/outline-narrowLine.html [ Failure ]
crbug.com/591099 fast/css/sticky/sticky-top-overflow-scroll-by-fragment.html [ Failure ]
crbug.com/591099 fast/css/text-overflow-ellipsis-vertical-hittest.html [ Failure ]
crbug.com/591099 fast/css/text-overflow-ellipsis-vertical-select.html [ Failure ]
crbug.com/591099 fast/css/transform-inline-style-remove.html [ Failure ]
......
......@@ -133,21 +133,17 @@ scoped_refptr<NGLayoutResult> NGLineBoxFragmentBuilder::ToLineBoxFragment() {
NGPhysicalSize physical_size = Size().ConvertToPhysical(line_writing_mode);
NGPhysicalOffsetRect contents_ink_overflow({}, physical_size);
NGPhysicalOffsetRect scrollable_overflow({}, physical_size);
for (size_t i = 0; i < children_.size(); ++i) {
NGPhysicalFragment* child = children_[i].get();
child->SetOffset(offsets_[i].ConvertToPhysical(
line_writing_mode, Direction(), physical_size, child->Size()));
child->PropagateContentsInkOverflow(&contents_ink_overflow);
NGPhysicalOffsetRect child_scroll_overflow = child->ScrollableOverflow();
child_scroll_overflow.offset += child->Offset();
scrollable_overflow.Unite(child_scroll_overflow);
}
scoped_refptr<NGPhysicalLineBoxFragment> fragment =
base::AdoptRef(new NGPhysicalLineBoxFragment(
Style(), style_variant_, physical_size, children_,
contents_ink_overflow, scrollable_overflow, metrics_, base_direction_,
contents_ink_overflow, metrics_, base_direction_,
break_token_ ? std::move(break_token_)
: NGInlineBreakToken::Create(node_)));
......
......@@ -5,6 +5,7 @@
#include "third_party/blink/renderer/core/layout/ng/inline/ng_physical_line_box_fragment.h"
#include "third_party/blink/renderer/core/layout/ng/inline/ng_inline_break_token.h"
#include "third_party/blink/renderer/core/layout/ng/ng_relative_utils.h"
#include "third_party/blink/renderer/core/style/computed_style.h"
namespace blink {
......@@ -15,7 +16,6 @@ NGPhysicalLineBoxFragment::NGPhysicalLineBoxFragment(
NGPhysicalSize size,
Vector<scoped_refptr<NGPhysicalFragment>>& children,
const NGPhysicalOffsetRect& contents_ink_overflow,
const NGPhysicalOffsetRect& scrollable_overflow,
const NGLineHeightMetrics& metrics,
TextDirection base_direction,
scoped_refptr<NGBreakToken> break_token)
......@@ -28,7 +28,6 @@ NGPhysicalLineBoxFragment::NGPhysicalLineBoxFragment(
children,
contents_ink_overflow,
std::move(break_token)),
scrollable_overflow_(scrollable_overflow),
metrics_(metrics) {
base_direction_ = static_cast<unsigned>(base_direction);
}
......@@ -45,6 +44,27 @@ NGPhysicalOffsetRect NGPhysicalLineBoxFragment::InkOverflow() const {
return ContentsInkOverflow();
}
NGPhysicalOffsetRect NGPhysicalLineBoxFragment::ScrollableOverflow(
const ComputedStyle* container_style,
NGPhysicalSize container_physical_size) const {
WritingMode container_writing_mode = container_style->GetWritingMode();
TextDirection container_direction = container_style->Direction();
NGPhysicalOffsetRect overflow({}, Size());
for (const auto& child : Children()) {
NGPhysicalOffsetRect child_scroll_overflow = child->ScrollableOverflow();
child_scroll_overflow.offset += child->Offset();
// If child has the same style as parent, parent will compute relative
// offset.
if (&child->Style() != container_style) {
child_scroll_overflow.offset +=
ComputeRelativeOffset(child->Style(), container_writing_mode,
container_direction, container_physical_size);
}
overflow.Unite(child_scroll_overflow);
}
return overflow;
}
const NGPhysicalFragment* NGPhysicalLineBoxFragment::FirstLogicalLeaf() const {
if (Children().IsEmpty())
return nullptr;
......
......@@ -21,7 +21,6 @@ class CORE_EXPORT NGPhysicalLineBoxFragment final
NGPhysicalSize size,
Vector<scoped_refptr<NGPhysicalFragment>>& children,
const NGPhysicalOffsetRect& contents_ink_overflow,
const NGPhysicalOffsetRect& scrollable_overflow,
const NGLineHeightMetrics&,
TextDirection base_direction,
scoped_refptr<NGBreakToken> break_token = nullptr);
......@@ -42,9 +41,12 @@ class CORE_EXPORT NGPhysicalLineBoxFragment final
NGPhysicalOffsetRect InkOverflow() const;
// Scrollable overflow. including contents, in the local coordinate.
NGPhysicalOffsetRect ScrollableOverflow() const {
return scrollable_overflow_;
}
// ScrollableOverflow is not precomputed/cached because it cannot be computed
// when LineBox is generated because it needs container dimensions to
// resolve relative position of its children.
NGPhysicalOffsetRect ScrollableOverflow(
const ComputedStyle* container_style,
NGPhysicalSize container_physical_size) const;
// Returns the first/last leaf fragment in the line in logical order. Returns
// nullptr if the line box is empty.
......@@ -58,11 +60,10 @@ class CORE_EXPORT NGPhysicalLineBoxFragment final
Vector<scoped_refptr<NGPhysicalFragment>> children_copy(children_);
return base::AdoptRef(new NGPhysicalLineBoxFragment(
Style(), StyleVariant(), size_, children_copy, contents_ink_overflow_,
scrollable_overflow_, metrics_, BaseDirection(), break_token_));
metrics_, BaseDirection(), break_token_));
}
private:
NGPhysicalOffsetRect scrollable_overflow_;
NGLineHeightMetrics metrics_;
};
......
......@@ -13,11 +13,13 @@
#include "third_party/blink/renderer/core/layout/layout_table_caption.h"
#include "third_party/blink/renderer/core/layout/layout_table_cell.h"
#include "third_party/blink/renderer/core/layout/ng/inline/ng_inline_node_data.h"
#include "third_party/blink/renderer/core/layout/ng/inline/ng_physical_line_box_fragment.h"
#include "third_party/blink/renderer/core/layout/ng/ng_constraint_space.h"
#include "third_party/blink/renderer/core/layout/ng/ng_layout_result.h"
#include "third_party/blink/renderer/core/layout/ng/ng_layout_utils.h"
#include "third_party/blink/renderer/core/layout/ng/ng_length_utils.h"
#include "third_party/blink/renderer/core/layout/ng/ng_physical_box_fragment.h"
#include "third_party/blink/renderer/core/layout/ng/ng_relative_utils.h"
#include "third_party/blink/renderer/core/page/scrolling/root_scroller_util.h"
#include "third_party/blink/renderer/core/paint/ng/ng_block_flow_painter.h"
#include "third_party/blink/renderer/core/paint/ng/ng_paint_fragment.h"
......@@ -72,9 +74,9 @@ void LayoutNGMixin<Base>::AddOverflowFromChildren() {
// |ComputeOverflow()| calls this, which is called from
// |CopyFragmentDataToLayoutBox()| and |RecalcOverflowAfterStyleChange()|.
// Add overflow from the last layout cycle.
if (Base::ChildrenInline()) {
if (const NGPhysicalBoxFragment* physical_fragment = CurrentFragment()) {
AddScrollingOverflowFromChildren();
if (const NGPhysicalBoxFragment* physical_fragment = CurrentFragment()) {
AddScrollingOverflowFromChildren();
if (Base::ChildrenInline()) {
Base::AddSelfVisualOverflow(
physical_fragment->SelfInkOverflow().ToLayoutFlippedRect(
physical_fragment->Style(), physical_fragment->Size()));
......@@ -94,6 +96,8 @@ void LayoutNGMixin<Base>::AddOverflowFromChildren() {
template <typename Base>
void LayoutNGMixin<Base>::AddScrollingOverflowFromChildren() {
bool children_inline = Base::ChildrenInline();
const NGPhysicalBoxFragment* physical_fragment = CurrentFragment();
DCHECK(physical_fragment);
// inline-end LayoutOverflow padding spec is still undecided:
......@@ -109,14 +113,31 @@ void LayoutNGMixin<Base>::AddScrollingOverflowFromChildren() {
}
NGPhysicalOffsetRect children_overflow;
for (const auto& child : physical_fragment->Children()) {
NGPhysicalOffsetRect child_scrollable_overflow =
child->ScrollableOverflow();
child_scrollable_overflow.offset += child->Offset();
if (child->IsLineBox() && padding_strut) {
child_scrollable_overflow.Expand(*padding_strut);
// Only add overflow for fragments NG has not reflected into Legacy.
// These fragments are:
// - inline fragments,
// - out of flow fragments whose css container is inline box.
// TODO(layout-dev) Transfroms also need to be applied to compute overflow
// correctly. NG is not yet transform-aware. crbug.com/855965
if (!physical_fragment->Children().IsEmpty()) {
for (const auto& child : physical_fragment->Children()) {
NGPhysicalOffsetRect child_scrollable_overflow;
if (child->IsOutOfFlowPositioned()) {
child_scrollable_overflow = child->ScrollableOverflow();
} else if (children_inline && child->IsLineBox()) {
DCHECK(child->IsLineBox());
child_scrollable_overflow =
ToNGPhysicalLineBoxFragment(*child).ScrollableOverflow(
Base::Style(), physical_fragment->Size());
if (padding_strut)
child_scrollable_overflow.Expand(*padding_strut);
} else {
continue;
}
child_scrollable_overflow.offset += child->Offset();
children_overflow.Unite(child_scrollable_overflow);
}
children_overflow.Unite(child_scrollable_overflow);
}
// LayoutOverflow takes flipped blocks coordinates, adjust as needed.
......
......@@ -337,7 +337,9 @@ NGPhysicalOffsetRect NGPhysicalFragment::ScrollableOverflow() const {
case NGPhysicalFragment::kFragmentText:
return {{}, Size()};
case NGPhysicalFragment::kFragmentLineBox:
return ToNGPhysicalLineBoxFragment(*this).ScrollableOverflow();
NOTREACHED()
<< "You must call NGLineBoxFragment::ScrollableOverflow explicitly.";
break;
}
NOTREACHED();
return {{}, Size()};
......
......@@ -5,7 +5,7 @@
#include "third_party/blink/renderer/core/layout/ng/ng_relative_utils.h"
#include "base/optional.h"
#include "third_party/blink/renderer/core/layout/ng/geometry/ng_logical_offset.h"
#include "third_party/blink/renderer/core/layout/ng/geometry/ng_physical_offset.h"
#include "third_party/blink/renderer/core/layout/ng/geometry/ng_physical_size.h"
#include "third_party/blink/renderer/core/style/computed_style.h"
#include "third_party/blink/renderer/platform/length_functions.h"
......@@ -13,13 +13,13 @@
namespace blink {
// Returns the child's relative position wrt the containing fragment.
NGLogicalOffset ComputeRelativeOffset(const ComputedStyle& child_style,
WritingMode container_writing_mode,
TextDirection container_direction,
NGLogicalSize container_logical_size) {
NGLogicalOffset offset;
NGPhysicalSize container_size =
container_logical_size.ConvertToPhysical(container_writing_mode);
NGPhysicalOffset ComputeRelativeOffset(const ComputedStyle& child_style,
WritingMode container_writing_mode,
TextDirection container_direction,
NGPhysicalSize container_size) {
NGPhysicalOffset offset;
if (child_style.GetPosition() != EPosition::kRelative)
return offset;
base::Optional<LayoutUnit> left, right, top, bottom;
......@@ -32,6 +32,10 @@ NGLogicalOffset ComputeRelativeOffset(const ComputedStyle& child_style,
if (!child_style.Bottom().IsAuto())
bottom = ValueForLength(child_style.Bottom(), container_size.height);
// Common case optimization
if (!left && !right && !top && !bottom)
return offset;
// Implements confict resolution rules from spec:
// https://www.w3.org/TR/css-position-3/#rel-pos
if (!left && !right) {
......@@ -51,26 +55,18 @@ NGLogicalOffset ComputeRelativeOffset(const ComputedStyle& child_style,
if (!bottom)
bottom = -top.value();
bool is_ltr = container_direction == TextDirection::kLtr;
switch (container_writing_mode) {
case WritingMode::kHorizontalTb:
offset.inline_offset = is_ltr ? left.value() : right.value();
offset.block_offset = top.value();
break;
case WritingMode::kVerticalRl:
case WritingMode::kSidewaysRl:
offset.inline_offset = is_ltr ? top.value() : bottom.value();
offset.block_offset = right.value();
break;
case WritingMode::kVerticalLr:
offset.inline_offset = is_ltr ? top.value() : bottom.value();
offset.block_offset = left.value();
break;
case WritingMode::kSidewaysLr:
offset.inline_offset = is_ltr ? bottom.value() : top.value();
offset.block_offset = left.value();
break;
if (IsHorizontalWritingMode(container_writing_mode)) {
if (IsLtr(container_direction))
offset.left = left.value();
else
offset.left = -right.value();
offset.top = top.value();
} else {
if (IsLtr(container_direction))
offset.top = top.value();
else
offset.top = -bottom.value();
offset.left = left.value();
}
return offset;
}
......
......@@ -12,16 +12,16 @@
namespace blink {
class ComputedStyle;
struct NGLogicalOffset;
struct NGPhysicalOffset;
// Implements relative positioning spec:
// https://www.w3.org/TR/css-position-3/#rel-pos
// Return relative position offset as defined by style.
CORE_EXPORT NGLogicalOffset
CORE_EXPORT NGPhysicalOffset
ComputeRelativeOffset(const ComputedStyle& child_style,
WritingMode container_writing_mode,
TextDirection container_direction,
NGLogicalSize container_size);
NGPhysicalSize container_size);
} // namespace blink
......
......@@ -5,14 +5,15 @@
#include "third_party/blink/renderer/core/layout/ng/ng_relative_utils.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/renderer/core/layout/ng/geometry/ng_logical_offset.h"
#include "third_party/blink/renderer/core/layout/ng/geometry/ng_physical_offset.h"
#include "third_party/blink/renderer/core/layout/ng/geometry/ng_physical_size.h"
#include "third_party/blink/renderer/core/style/computed_style.h"
namespace blink {
namespace {
const LayoutUnit kInlineSize{100};
const LayoutUnit kBlockSize{200};
const LayoutUnit kHorizontalSize{100};
const LayoutUnit kVerticalSize{200};
const LayoutUnit kLeft{3};
const LayoutUnit kRight{5};
const LayoutUnit kTop{7};
......@@ -24,7 +25,8 @@ class NGRelativeUtilsTest : public testing::Test {
protected:
void SetUp() override {
style_ = ComputedStyle::Create();
container_size_ = NGLogicalSize{kInlineSize, kBlockSize};
style_->SetPosition(EPosition::kRelative);
container_size_ = NGPhysicalSize{kHorizontalSize, kVerticalSize};
}
void SetTRBL(LayoutUnit top,
......@@ -44,18 +46,18 @@ class NGRelativeUtilsTest : public testing::Test {
}
scoped_refptr<ComputedStyle> style_;
NGLogicalSize container_size_;
NGPhysicalSize container_size_;
};
TEST_F(NGRelativeUtilsTest, HorizontalTB) {
NGLogicalOffset offset;
NGPhysicalOffset offset;
// Everything auto defaults to kZero,kZero
SetTRBL(kAuto, kAuto, kAuto, kAuto);
offset = ComputeRelativeOffset(*style_, WritingMode::kHorizontalTb,
TextDirection::kLtr, container_size_);
EXPECT_EQ(offset.inline_offset, kZero);
EXPECT_EQ(offset.block_offset, kZero);
EXPECT_EQ(offset.left, kZero);
EXPECT_EQ(offset.top, kZero);
// Set all sides
SetTRBL(kTop, kRight, kBottom, kLeft);
......@@ -63,25 +65,25 @@ TEST_F(NGRelativeUtilsTest, HorizontalTB) {
// kLtr
offset = ComputeRelativeOffset(*style_, WritingMode::kHorizontalTb,
TextDirection::kLtr, container_size_);
EXPECT_EQ(offset.inline_offset, kLeft);
EXPECT_EQ(offset.block_offset, kTop);
EXPECT_EQ(offset.left, kLeft);
EXPECT_EQ(offset.top, kTop);
// kRtl
offset = ComputeRelativeOffset(*style_, WritingMode::kHorizontalTb,
TextDirection::kRtl, container_size_);
EXPECT_EQ(offset.inline_offset, kRight);
EXPECT_EQ(offset.block_offset, kTop);
EXPECT_EQ(offset.left, -kRight);
EXPECT_EQ(offset.top, kTop);
// Set only non-default sides
SetTRBL(kAuto, kRight, kBottom, kAuto);
offset = ComputeRelativeOffset(*style_, WritingMode::kHorizontalTb,
TextDirection::kLtr, container_size_);
EXPECT_EQ(offset.inline_offset, -kRight);
EXPECT_EQ(offset.block_offset, -kBottom);
EXPECT_EQ(offset.left, -kRight);
EXPECT_EQ(offset.top, -kBottom);
}
TEST_F(NGRelativeUtilsTest, VerticalRightLeft) {
NGLogicalOffset offset;
NGPhysicalOffset offset;
// Set all sides
SetTRBL(kTop, kRight, kBottom, kLeft);
......@@ -89,25 +91,25 @@ TEST_F(NGRelativeUtilsTest, VerticalRightLeft) {
// kLtr
offset = ComputeRelativeOffset(*style_, WritingMode::kVerticalRl,
TextDirection::kLtr, container_size_);
EXPECT_EQ(offset.inline_offset, kTop);
EXPECT_EQ(offset.block_offset, kRight);
EXPECT_EQ(offset.top, kTop);
EXPECT_EQ(offset.left, kLeft);
// kRtl
offset = ComputeRelativeOffset(*style_, WritingMode::kVerticalRl,
TextDirection::kRtl, container_size_);
EXPECT_EQ(offset.inline_offset, kBottom);
EXPECT_EQ(offset.block_offset, kRight);
EXPECT_EQ(offset.top, -kBottom);
EXPECT_EQ(offset.left, kLeft);
// Set only non-default sides
SetTRBL(kAuto, kAuto, kBottom, kLeft);
SetTRBL(kAuto, kRight, kBottom, kAuto);
offset = ComputeRelativeOffset(*style_, WritingMode::kVerticalRl,
TextDirection::kLtr, container_size_);
EXPECT_EQ(offset.inline_offset, -kBottom);
EXPECT_EQ(offset.block_offset, -kLeft);
EXPECT_EQ(offset.top, -kBottom);
EXPECT_EQ(offset.left, -kRight);
}
TEST_F(NGRelativeUtilsTest, VerticalLeftRight) {
NGLogicalOffset offset;
NGPhysicalOffset offset;
// Set all sides
SetTRBL(kTop, kRight, kBottom, kLeft);
......@@ -115,21 +117,21 @@ TEST_F(NGRelativeUtilsTest, VerticalLeftRight) {
// kLtr
offset = ComputeRelativeOffset(*style_, WritingMode::kVerticalLr,
TextDirection::kLtr, container_size_);
EXPECT_EQ(offset.inline_offset, kTop);
EXPECT_EQ(offset.block_offset, kLeft);
EXPECT_EQ(offset.top, kTop);
EXPECT_EQ(offset.left, kLeft);
// kRtl
offset = ComputeRelativeOffset(*style_, WritingMode::kVerticalLr,
TextDirection::kRtl, container_size_);
EXPECT_EQ(offset.inline_offset, kBottom);
EXPECT_EQ(offset.block_offset, kLeft);
EXPECT_EQ(offset.top, -kBottom);
EXPECT_EQ(offset.left, kLeft);
// Set only non-default sides
SetTRBL(kAuto, kRight, kBottom, kAuto);
offset = ComputeRelativeOffset(*style_, WritingMode::kVerticalLr,
TextDirection::kLtr, container_size_);
EXPECT_EQ(offset.inline_offset, -kBottom);
EXPECT_EQ(offset.block_offset, -kRight);
EXPECT_EQ(offset.top, -kBottom);
EXPECT_EQ(offset.left, -kRight);
}
} // namespace
......
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