LibWeb: Compute scrollable overflow via containing block instead of children #1268

ebanner · 2024-09-03T15:49:55Z

Instead of using child boxes to compute scrollable overflow for the box, we use descendants which have the box as their containing block.

Fixes #718

Rebaselined Tests

ebanner · 2024-09-06T17:55:22Z

This PR causes a whole bunch of tests to break. Looking at them, it's not clear to me that the tests are correct, though.

Most (all?) of the tests that break essentially look like this:

Test failed: file:///Users/edward/Code/ladybird/Tests/LibWeb/Layout/input/table/clip-spans-to-table-end.html
205/1304: Layout/input/table/table-cellpadd--- /Users/edward/Code/ladybird/Tests/LibWeb/Layout/expected/./table/table-cellpadding.txt
+++ /Users/edward/Code/ladybird/Tests/LibWeb/Layout/expected/./table/table-cellpadding.txt
@@ -25,8 +25,8 @@
     PaintableWithLines (BlockContainer<BODY>) [8,8 784x204]
       PaintableWithLines (TableWrapper(anonymous)) [8,8 498.1875x204]
         PaintableBox (Box<TABLE>) [8,8 498.1875x204]
-          PaintableBox (Box<TBODY>) [9,9 488.1875x198] overflow: [9,9 494.1875x200]
-            PaintableBox (Box<TR>) [11,11 488.1875x198] overflow: [11,11 492.1875x198]
+          PaintableBox (Box<TBODY>) [9,9 488.1875x198]
+            PaintableBox (Box<TR>) [11,11 488.1875x198]
               PaintableWithLines (BlockContainer<TD>) [11,11 146.640625x198]
                 TextPaintable (TextNode<#text>)
               PaintableWithLines (BlockContainer<TD>) [159.640625,11 165.4375x198]

That is, the overflow is removed.

I'm reasonably sure there should be no overflow though. In this test for instance, TBODY and TR and not containing blocks, so they can't have any overflow (is that right?). Also, Chrome doesn't compute any overflow for TBODY nor TR. You can verify this by adding:

tbody, tr {
    overflow: scroll;
}

and seeing that Chrome does not display any scrollbars (whereas ladybird does).

I looked at a few more of the table tests and they follow this pattern and can be verified by setting overflow: scroll. There are other tests like block-and-inline where overflow is present on HTML and BODY, but in most cases HTML and BODY are not containing blocks. I also looked at some of the flex tests and a similar thing follows for the DIVs (where they are mostly not containing blocks but are getting overflow).

kalenikaliaksandr · 2024-09-06T18:42:40Z

I'm reasonably sure there should be no overflow though. In this test for instance, TBODY and TR and not containing blocks, so they can't have any overflow (is that right?). Also, Chrome doesn't compute any overflow for TBODY nor TR. You can verify this by adding:

you are right, in this specific example you provided, removed overflow is improvement. so you would have to simply update a test expectation in that case. though I bet there might be some valid regressions, particularly when CSS transform is involved. the last time I attempted to make similar change, I discovered that if we don't also resolve this FIXME, it will cause some regression in the ref-tests.
FIXME: accounting for transforms by projecting each box onto the plane of the element that establishes its 3D rendering context. [CSS3-TRANSFORMS]

ebanner · 2024-09-25T14:02:10Z

Most (all?) of the table tests are failing because <TBODY> are getting overflow when they presumably shouldn't be: #1525

ebanner · 2024-10-18T13:09:03Z

@kalenikaliaksandr I don't see any failures in the ref-tests. That means we don't have to address that FIXME for this issue..?

kalenikaliaksandr · 2024-10-18T13:11:32Z

@kalenikaliaksandr I don't see any failures in the ref-tests. That means we don't have to address that FIXME for this issue..?

what FIXME do you mean exactly? btw please rebase your branch.

ebanner · 2024-10-18T13:12:56Z

I was responding to this part of your comment:

though I bet there might be some valid regressions, particularly when CSS transform is involved. the last time I attempted to make similar change, I discovered that if we don't also resolve this FIXME, it will cause some regression in the ref-tests.
FIXME: accounting for transforms by projecting each box onto the plane of the element that establishes its 3D rendering context. [CSS3-TRANSFORMS]

kalenikaliaksandr · 2024-10-18T13:14:36Z

I was responding to this part of your comment:

though I bet there might be some valid regressions, particularly when CSS transform is involved. the last time I attempted to make similar change, I discovered that if we don't also resolve this FIXME, it will cause some regression in the ref-tests.
FIXME: accounting for transforms by projecting each box onto the plane of the element that establishes its 3D rendering context. [CSS3-TRANSFORMS]

yes, if this change does not break any tests then it's fine to leave the FIXME.

ebanner · 2024-10-18T13:17:06Z

I'm not quite sure what you mean by rebase my branch. Rebase it off of..? I don't see anything that looks off.

kalenikaliaksandr · 2024-10-18T13:22:02Z

I'm not quite sure what you mean by rebase my branch. Rebase it off of..? I don't see anything that looks off.

I mean rebase onto master branch to include latest changes from there by doing git rebase master.

Instead of using child boxes to compute scrollable overflow for the box, we use descendants which have the box as their containing block.

kalenikaliaksandr

I tested change locally and it seems good. likely it's going to result in overflow measuring being heavy in profiles because we will do full subtree traversal for each child, but that could be fixed.

ebanner · 2024-10-18T13:24:45Z

ok done 👍

(btw I'm curious what you saw on the PR that led you to ask me to rebase off master)

kalenikaliaksandr · 2024-10-18T13:26:20Z

ok done 👍

(btw I'm curious what you saw on the PR that led you to ask me to rebase off master)

I seen InlinePaintable in text expectations which is gone by now, so I was a bit confused. apparently it does not introduce any conflicts so that's fine.

ebanner · 2024-10-18T13:34:41Z

I tested change locally and it seems good. likely it's going to result in overflow measuring being heavy in profiles because we will do full subtree traversal for each child, but that could be fixed.

What are you thinking for a fix? The most obvious one that jumps out to me is to have a box point to all the boxes for which it's the containing block (so it doesn't have to traverse the whole subtree every time to find them).

kalenikaliaksandr · 2024-10-18T13:37:54Z

I tested change locally and it seems good. likely it's going to result in overflow measuring being heavy in profiles because we will do full subtree traversal for each child, but that could be fixed.

What are you thinking for a fix? The most obvious one that jumps out to me is to have a box point to all the boxes for which it's the containing block (so it doesn't have to traverse the whole subtree every time to find them).

yeah, I think it's either having a linked list of all contained abspos boxes for each paintable. Or, better idea: diverging paintable tree from layout tree by and making contained abspos boxes to be direct children of containing block.

ebanner · 2024-10-18T16:37:49Z

Or, better idea: diverging paintable tree from layout tree by and making contained abspos boxes to be direct children of containing block.

That sounds like a cool idea.

The other case where the containing block comes into play that I came across (in addition to abspos boxes) is tables. All table sub-elements have the TableWrapper as its containing block (so a TD won't cause overflow on a TR for example).

Do you imagine it working the same way for tables? That is, the TableWrapper would have TBODY, TR, TD etc. as its children in the paintable tree?

ebanner marked this pull request as draft September 3, 2024 15:50

ebanner force-pushed the fix-scrollable-overflow branch 3 times, most recently from 015f664 to dcb88f0 Compare September 3, 2024 18:18

ebanner changed the title ~~Layout: Use containing block to compute scrollable overflow~~ LibWeb: Use containing block to compute scrollable overflow Sep 3, 2024

ebanner force-pushed the fix-scrollable-overflow branch 4 times, most recently from b7d0c87 to 44d434b Compare September 4, 2024 13:11

ebanner mentioned this pull request Sep 25, 2024

<TBODY> are getting superfluous overflow #1525

Closed

ebanner mentioned this pull request Oct 14, 2024

LibWeb: Fix table overflow issues #1542

Merged

48 tasks

ebanner force-pushed the fix-scrollable-overflow branch 2 times, most recently from db1bd62 to df34562 Compare October 15, 2024 14:28

ebanner closed this Oct 15, 2024

ebanner force-pushed the fix-scrollable-overflow branch from df34562 to 81596b4 Compare October 15, 2024 14:31

ebanner reopened this Oct 15, 2024

ebanner force-pushed the fix-scrollable-overflow branch 4 times, most recently from 2f1ba7b to 5dbd93f Compare October 17, 2024 20:41

ebanner changed the title ~~LibWeb: Use containing block to compute scrollable overflow~~ LibWeb: Compute scrollable overflow via containing block not children Oct 17, 2024

ebanner changed the title ~~LibWeb: Compute scrollable overflow via containing block not children~~ LibWeb: Compute scrollable overflow via containing block instead of children Oct 17, 2024

ebanner force-pushed the fix-scrollable-overflow branch 2 times, most recently from 34441d0 to b2a7f40 Compare October 17, 2024 20:49

ebanner marked this pull request as ready for review October 18, 2024 13:09

LibWeb: Use containing block to compute scrollable overflow

4df45b4

Instead of using child boxes to compute scrollable overflow for the box, we use descendants which have the box as their containing block.

ebanner force-pushed the fix-scrollable-overflow branch from b2a7f40 to 4df45b4 Compare October 18, 2024 13:23

kalenikaliaksandr approved these changes Oct 18, 2024

View reviewed changes

kalenikaliaksandr merged commit 912511a into LadybirdBrowser:master Oct 18, 2024
6 checks passed

This was referenced Oct 25, 2024

LibWeb: Use cached contained elements when computing scrollable overflow #1964

Draft

LibWeb: Cache contained elements in measure_scrollable_overflow() ebanner/ladybird#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LibWeb: Compute scrollable overflow via containing block instead of children #1268

LibWeb: Compute scrollable overflow via containing block instead of children #1268

ebanner commented Sep 3, 2024 •

edited

Loading

ebanner commented Sep 6, 2024 •

edited

Loading

kalenikaliaksandr commented Sep 6, 2024

ebanner commented Sep 25, 2024

ebanner commented Oct 18, 2024

kalenikaliaksandr commented Oct 18, 2024

ebanner commented Oct 18, 2024

kalenikaliaksandr commented Oct 18, 2024

ebanner commented Oct 18, 2024

kalenikaliaksandr commented Oct 18, 2024

kalenikaliaksandr left a comment

ebanner commented Oct 18, 2024

kalenikaliaksandr commented Oct 18, 2024

ebanner commented Oct 18, 2024 •

edited

Loading

kalenikaliaksandr commented Oct 18, 2024

ebanner commented Oct 18, 2024 •

edited

Loading

LibWeb: Compute scrollable overflow via containing block instead of children #1268

LibWeb: Compute scrollable overflow via containing block instead of children #1268

Conversation

ebanner commented Sep 3, 2024 • edited Loading

Rebaselined Tests

ebanner commented Sep 6, 2024 • edited Loading

kalenikaliaksandr commented Sep 6, 2024

ebanner commented Sep 25, 2024

ebanner commented Oct 18, 2024

kalenikaliaksandr commented Oct 18, 2024

ebanner commented Oct 18, 2024

kalenikaliaksandr commented Oct 18, 2024

ebanner commented Oct 18, 2024

kalenikaliaksandr commented Oct 18, 2024

kalenikaliaksandr left a comment

Choose a reason for hiding this comment

ebanner commented Oct 18, 2024

kalenikaliaksandr commented Oct 18, 2024

ebanner commented Oct 18, 2024 • edited Loading

kalenikaliaksandr commented Oct 18, 2024

ebanner commented Oct 18, 2024 • edited Loading

ebanner commented Sep 3, 2024 •

edited

Loading

ebanner commented Sep 6, 2024 •

edited

Loading

ebanner commented Oct 18, 2024 •

edited

Loading

ebanner commented Oct 18, 2024 •

edited

Loading