`list-style-position` doesn't seem to work?

LadybirdBrowser / ladybird

Truly independent web browser

https://ladybird.org

BSD 2-Clause "Simplified" License

20.94k stars 886 forks source link

`list-style-position` doesn't seem to work? #1645

Open pavpanchekha opened 1 week ago

pavpanchekha commented 1 week ago

I reduced a layout issue on https://herbie.uwplse.org/ to the following code block:

<!doctype html>
<style>
ul {
    width: 1rem;
    border: 1px solid black;
    list-style-position: inside;
}
</style>
<ul>
  <li>Text</li>
</ul>

The Chrome rendering looks like this:

The 1rem width is too small to fit the content, which forces the maximum number of line breaks. Because the list has list-style-position: inside, the marker (bullet) is the first inline box in the <li> and therefore the first line is just the bullet and the second line is the word. Here's the standard text:

The marker box is placed as the first inline box in the principal block box, before the element's content and before any :before pseudo-elements. CSS 2.1 does not specify the precise location of the marker box.

Here's the Ladybird rendering:

It's not just a line breaking issue; if I add a second word to the HTML you get this:

pavpanchekha commented 1 week ago

I'm going to try to poke around and see if I can figure this out.

pavpanchekha commented 1 week ago

First hint, this FIXME in BlockFormattingContext::compute_auto_height_for_block_level_element:

FIXME: This is hack. If the last child is a list-item marker box, we ignore it for purposes of height calculation. Perhaps markers should not be considered in-flow(?) Perhaps they should always be the first child of the list-item box instead of the last child.

The marker is indeed the last child, which is pretty weird, but I guess I'm still looking for the layout code itself.

pavpanchekha commented 1 week ago

Here's the code handling inside position:

    if (marker.list_style_position() == CSS::ListStylePosition::Inside) {
        list_item_state.set_content_offset({ final_marker_width, list_item_state.offset.y() });
        list_item_state.set_content_width(list_item_state.content_width() - final_marker_width);
    }

This doesn't make a lot of sense to me and seems more like the Outside behavior. I'll keep digging.

pavpanchekha commented 1 week ago

Ok, I do think this is the guilty line of code. I think a fix would look like this:

Place the ListItemMarker at the start, not end, of the parent, at least for inside position
Run the current code only for outside position
For inside position lay it out like an inline element with nothing else special happening
If the ListItemMarker's location in its parent depends on its position, remember to invalidate the layout tree if the position changes; it's probably better to put it always at the front.

pavpanchekha commented 1 week ago

Ok, a broader issue seems to be that ListItemBox is a BlockContainer, which is correct in general I think, but with list-style-position: inside it seems to generate inline children too, which means we need to create an anonymous block box inside it first, to hold the marker. Seems to be what Chrome does (reverse engineering only, not looking at code). But if there's already an anonymous block box inside, then we want to stick our marker inside that.

pavpanchekha commented 1 week ago

Yeah I think unfortunately tree-building has to read the list item position. I did look and Blink does it this way too (it has different marker classes for the different list style positions).