From ddbfd77e2c0bedc49ee9c0cf0620f326bf470a68 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Thu, 26 Nov 2020 21:18:34 +0100 Subject: [PATCH] LibWeb: Don't put block boxes inside inlines Inline layout nodes cannot have block children (except inline-block, of course.) When encountering a block box child of an inline, we now hoist the block up to the inline's containing block, and also wrap any preceding inline siblings in an anonymous wrapper block. This improves the ACID2 situation quite a bit (although we still need floats to really bring it home.) I also took this opportunity to move all tree building logic into Layout::TreeBuilder, to continue the theme of absolving our LayoutNode objects of responsibilities. :^) --- Libraries/LibWeb/Layout/BlockBox.cpp | 21 ---- Libraries/LibWeb/Layout/BlockBox.h | 4 - Libraries/LibWeb/Layout/Node.cpp | 1 + Libraries/LibWeb/Layout/Node.h | 2 - Libraries/LibWeb/Layout/TreeBuilder.cpp | 129 ++++++++++++++++++------ Libraries/LibWeb/Layout/TreeBuilder.h | 12 ++- 6 files changed, 108 insertions(+), 61 deletions(-) diff --git a/Libraries/LibWeb/Layout/BlockBox.cpp b/Libraries/LibWeb/Layout/BlockBox.cpp index 54799bcbb4..9bd199fb93 100644 --- a/Libraries/LibWeb/Layout/BlockBox.cpp +++ b/Libraries/LibWeb/Layout/BlockBox.cpp @@ -47,15 +47,6 @@ BlockBox::~BlockBox() { } -Node& BlockBox::inline_wrapper() -{ - if (!last_child() || !last_child()->is_block() || last_child()->dom_node() != nullptr) { - append_child(adopt(*new BlockBox(document(), nullptr, style_for_anonymous_block()))); - last_child()->set_children_are_inline(true); - } - return *last_child(); -} - void BlockBox::paint(PaintContext& context, PaintPhase phase) { if (!is_visible()) @@ -119,18 +110,6 @@ HitTestResult BlockBox::hit_test(const Gfx::IntPoint& position, HitTestType type return { absolute_rect().contains(position.x(), position.y()) ? this : nullptr }; } -NonnullRefPtr BlockBox::style_for_anonymous_block() const -{ - auto new_style = CSS::StyleProperties::create(); - - specified_style().for_each_property([&](auto property_id, auto& value) { - if (CSS::StyleResolver::is_inherited_property(property_id)) - new_style->set_property(property_id, value); - }); - - return new_style; -} - void BlockBox::split_into_lines(BlockBox& container, LayoutMode layout_mode) { auto* line_box = &container.ensure_last_line_box(); diff --git a/Libraries/LibWeb/Layout/BlockBox.h b/Libraries/LibWeb/Layout/BlockBox.h index 4234dcd39a..8d1b43d3a8 100644 --- a/Libraries/LibWeb/Layout/BlockBox.h +++ b/Libraries/LibWeb/Layout/BlockBox.h @@ -40,8 +40,6 @@ public: virtual void paint(PaintContext&, PaintPhase) override; - virtual Node& inline_wrapper() override; - virtual HitTestResult hit_test(const Gfx::IntPoint&, HitTestType) const override; BlockBox* previous_sibling() { return downcast(Node::previous_sibling()); } @@ -58,8 +56,6 @@ public: private: virtual bool is_block() const override { return true; } - - NonnullRefPtr style_for_anonymous_block() const; }; template diff --git a/Libraries/LibWeb/Layout/Node.cpp b/Libraries/LibWeb/Layout/Node.cpp index b843fbc3d5..fa196022b3 100644 --- a/Libraries/LibWeb/Layout/Node.cpp +++ b/Libraries/LibWeb/Layout/Node.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include diff --git a/Libraries/LibWeb/Layout/Node.h b/Libraries/LibWeb/Layout/Node.h index 93bac6ca39..4725ecbdf8 100644 --- a/Libraries/LibWeb/Layout/Node.h +++ b/Libraries/LibWeb/Layout/Node.h @@ -134,8 +134,6 @@ public: bool can_contain_boxes_with_position_absolute() const; - virtual Node& inline_wrapper() { return *this; } - const CSS::StyleProperties& specified_style() const; const ImmutableLayoutStyle& style() const; diff --git a/Libraries/LibWeb/Layout/TreeBuilder.cpp b/Libraries/LibWeb/Layout/TreeBuilder.cpp index 7b20178093..bbcb690279 100644 --- a/Libraries/LibWeb/Layout/TreeBuilder.cpp +++ b/Libraries/LibWeb/Layout/TreeBuilder.cpp @@ -37,53 +37,116 @@ TreeBuilder::TreeBuilder() { } -static RefPtr create_layout_tree(DOM::Node& node, const CSS::StyleProperties* parent_style) +static NonnullRefPtr style_for_anonymous_block(Node& parent_box) { - auto layout_node = node.create_layout_node(parent_style); - if (!layout_node) - return nullptr; + auto new_style = CSS::StyleProperties::create(); - if (!node.has_children()) - return layout_node; - - NonnullRefPtrVector layout_children; - bool have_inline_children = false; - bool have_noninline_children = false; - - downcast(node).for_each_child([&](DOM::Node& child) { - auto layout_child = create_layout_tree(child, &layout_node->specified_style()); - if (!layout_child) - return; - if (layout_child->is_inline()) - have_inline_children = true; - else - have_noninline_children = true; - layout_children.append(layout_child.release_nonnull()); + parent_box.specified_style().for_each_property([&](auto property_id, auto& value) { + if (CSS::StyleResolver::is_inherited_property(property_id)) + new_style->set_property(property_id, value); }); - for (auto& layout_child : layout_children) { - if (have_noninline_children && have_inline_children && layout_child.is_inline()) { - if (is(layout_child) && downcast(layout_child).text_for_style(*parent_style) == " ") - continue; - layout_node->inline_wrapper().append_child(layout_child); + return new_style; +} + +static Layout::Node& insertion_parent_for_inline_node(Layout::Node& layout_parent, Layout::Node& layout_node) +{ + if (layout_parent.is_inline()) + return layout_parent; + + if (!layout_parent.has_children() || layout_parent.children_are_inline()) + return layout_parent; + + // layout_parent has block children, insert into an anonymous wrapper block (and create it first if needed) + if (!layout_parent.last_child()->is_anonymous() || !layout_parent.last_child()->children_are_inline()) { + layout_parent.append_child(adopt(*new BlockBox(layout_node.document(), nullptr, style_for_anonymous_block(layout_parent)))); + } + return *layout_parent.last_child(); +} + +static Layout::Node& insertion_parent_for_block_node(Layout::Node& layout_parent, Layout::Node& layout_node) +{ + if (!layout_parent.has_children() || !layout_parent.children_are_inline()) + return layout_parent; + + // layout_parent has inline children, first move them into an anonymous wrapper block + if (layout_parent.children_are_inline()) { + NonnullRefPtrVector children; + while (RefPtr child = layout_parent.first_child()) { + layout_parent.remove_child(*child); + children.append(child.release_nonnull()); + } + layout_parent.append_child(adopt(*new BlockBox(layout_node.document(), nullptr, style_for_anonymous_block(layout_parent)))); + layout_parent.set_children_are_inline(false); + for (auto& child : children) { + layout_parent.last_child()->append_child(child); + } + layout_parent.last_child()->set_children_are_inline(true); + } + + if (!layout_parent.last_child()->is_anonymous() || layout_parent.last_child()->children_are_inline()) { + layout_parent.append_child(adopt(*new BlockBox(layout_node.document(), nullptr, style_for_anonymous_block(layout_parent)))); + } + return *layout_parent.last_child(); +} + +void TreeBuilder::create_layout_tree(DOM::Node& dom_node) +{ + // If the parent doesn't have a layout node, we don't need one either. + if (dom_node.parent() && !dom_node.parent()->layout_node()) + return; + + const CSS::StyleProperties* parent_style = m_parent_stack.is_empty() ? nullptr : &m_parent_stack.last()->specified_style(); + + auto layout_node = dom_node.create_layout_node(parent_style); + if (!layout_node) + return; + + // Discard empty whitespace nodes. This might not be ideal for correctness, but it does make the tree nicer. + if (is(*layout_node) && downcast(*layout_node).text_for_style(*parent_style) == " ") + return; + + if (!dom_node.parent()) { + m_layout_root = layout_node; + } else { + if (layout_node->is_inline()) { + // Inlines can be inserted into the nearest ancestor. + auto& insertion_point = insertion_parent_for_inline_node(*m_parent_stack.last(), *layout_node); + insertion_point.append_child(*layout_node); + insertion_point.set_children_are_inline(true); } else { - layout_node->append_child(layout_child); + // Blocks can't be inserted into an inline parent, so find the nearest block ancestor. + auto& nearest_block_ancestor = [&]() -> Layout::Node& { + for (ssize_t i = m_parent_stack.size() - 1; i >= 0; --i) { + if (m_parent_stack[i]->is_block()) + return *m_parent_stack[i]; + } + ASSERT_NOT_REACHED(); + }(); + auto& insertion_point = insertion_parent_for_block_node(nearest_block_ancestor, *layout_node); + insertion_point.append_child(*layout_node); + insertion_point.set_children_are_inline(false); } } - if (have_inline_children && !have_noninline_children) - layout_node->set_children_are_inline(true); - - return layout_node; + if (dom_node.has_children()) { + push_parent(*layout_node); + downcast(dom_node).for_each_child([&](auto& dom_child) { + create_layout_tree(dom_child); + }); + pop_parent(); + } } -RefPtr TreeBuilder::build(DOM::Node& node) +RefPtr TreeBuilder::build(DOM::Node& dom_node) { - if (!is(node) && node.has_children()) { + if (!is(dom_node) && dom_node.has_children()) { dbg() << "FIXME: Support building partial layout trees."; return nullptr; } - return create_layout_tree(node, nullptr); + + create_layout_tree(dom_node); + return move(m_layout_root); } } diff --git a/Libraries/LibWeb/Layout/TreeBuilder.h b/Libraries/LibWeb/Layout/TreeBuilder.h index 23da674584..40c79bb3ad 100644 --- a/Libraries/LibWeb/Layout/TreeBuilder.h +++ b/Libraries/LibWeb/Layout/TreeBuilder.h @@ -26,6 +26,7 @@ #pragma once +#include #include #include @@ -35,7 +36,16 @@ class TreeBuilder { public: TreeBuilder(); - RefPtr build(DOM::Node&); + RefPtr build(DOM::Node&); + +private: + void create_layout_tree(DOM::Node&); + + void push_parent(Layout::Node& node) { m_parent_stack.append(&node); } + void pop_parent() { m_parent_stack.take_last(); } + + RefPtr m_layout_root; + Vector m_parent_stack; }; }