Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LibWeb: Correctly lay out block elements inside inline elements #3276

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gmta
Copy link
Collaborator

@gmta gmta commented Jan 16, 2025

The contents of inline elements containing a block element were incorrectly ordered, e.g.

<b>foo<div>bar</div>baz</b>

...did not render the <div> as being in between the foo and baz fragments.

Before After
Screenshot_20250116_164644 image

Excerpt from Web::Layout::TreeBuilder:


Block nodes inside inline nodes are allowed, but to maintain the invariant that either all layout children are
inline or non-inline, we need to rearrange the tree a bit. All inline ancestors up to the node we've inserted are
wrapped in an anonymous block, which is inserted into the nearest non-inline ancestor. We then recreate the inline
ancestors in another anonymous block inserted after the node so we can continue adding children.

Effectively, we try to turn this:

InlineNode 1
  TextNode 1
  InlineNode N
    TextNode N
    BlockContainer (node)

Into this:

BlockContainer (anonymous "before")
  InlineNode 1
    TextNode 1
    InlineNode N
      TextNode N
BlockContainer (anonymous "middle") continuation
  BlockContainer (node)
BlockContainer (anonymous "after")
  InlineNode 1 continuation
    InlineNode N

To be able to reconstruct their relation after restructuring, layout nodes keep track of their continuation. The
top-most inline node of the "after" wrapper points to the "middle" wrapper, which points to the top-most inline node
of the "before" wrapper. All other inline nodes in the "after" wrapper point to their counterparts in the "before"
wrapper, to make it easier to create the right paintables since a DOM::Node only has a single Layout::Node.

Appending then continues in the "after" tree. If a new block node is then inserted, we can reuse the "middle" wrapper
if no inline siblings exist for node or its ancestors, and leave the existing "after" wrapper alone. Otherwise, we
create new wrappers and extend the continuation chain.

Inspired by: https://webkit.org/blog/115/webcore-rendering-ii-blocks-and-inlines/

@gmta gmta force-pushed the libweb-block-elements-inside-inline-elements branch 4 times, most recently from 92e2698 to e3732e8 Compare January 16, 2025 15:56
@gmta gmta changed the title LibWeb: Correctly order block elements inside inline elements LibWeb: Correctly lay out block elements inside inline elements Jan 16, 2025
Copy link
Member

@kalenikaliaksandr kalenikaliaksandr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you considered solving the problem on paintable tree level? I suppose webkit has to use continuations on layout tree because they do not have paintable tree concept, while we do and support 1:N relationship between layout and paintable nodes.

Libraries/LibWeb/Layout/Node.h Outdated Show resolved Hide resolved
@gmta gmta requested a review from awesomekling January 16, 2025 16:20
@gmta gmta force-pushed the libweb-block-elements-inside-inline-elements branch 3 times, most recently from 6d662fe to 1f2ee7e Compare January 17, 2025 10:56
gmta added 2 commits January 17, 2025 12:43
This allows for easy child removal similar to `DOM::Node::remove()`.
The existing `::unite_horizontally()` and `::unite_vertically()` tests
did not properly test the edge cases where left/top in the Rect were
updated, so they get re-arranged a bit.
@gmta gmta force-pushed the libweb-block-elements-inside-inline-elements branch from 1f2ee7e to 49dd2f9 Compare January 17, 2025 11:49
Our layout tree requires that all containers either have inline or
non-inline children. In order to support the layout of non-inline
elements inside inline elements, we need to do a bit of tree
restructuring. It effectively simulates temporarily closing all inline
nodes, appending the block element, and resumes appending to the last
open inline node.

The acid1.txt expectation needed to be updated to reflect the fact that
we now hoist its <p> elements out of the inline <form> they were in.
Visually, the before and after situations for acid1.html are identical.
@gmta gmta force-pushed the libweb-block-elements-inside-inline-elements branch from 49dd2f9 to 827da96 Compare January 17, 2025 12:11
@gmta
Copy link
Collaborator Author

gmta commented Jan 17, 2025

@kalenikaliaksandr @awesomekling I've rearranged some things:

  • Determining the united border/padding/margin/content rects is now the responsibility of PaintableBox again.
  • The continuation chain is still only part of the layout tree however; it's not clear to me if we're always going to have paintables for parts of split-up inline elements that might not be visible (e.g. in <b><div>foo</div>bar</b>) and if so, how we're going to reliably link up the right paintables during creation of the painting tree. I've left a FIXME in PaintableBox to this end.
  • More DOM methods were updated and I've added a test for offsetTop/Left/Width/Height.

Copy link
Member

@kalenikaliaksandr kalenikaliaksandr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we add a test for hit-testing just so we can be sure it works as expected when it comes to continuations?

@gmta
Copy link
Collaborator Author

gmta commented Jan 17, 2025

could we add a test for hit-testing just so we can be sure it works as expected when it comes to continuations?

Absolutely, in fact, I'm also working on a bugfix for something depending on this that involves hit-testing. I'll tack on the commit to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants