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.

Sign up for GitHub

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

Jump to bottom

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

Open
pavpanchekha opened this issue Oct 6, 2024 · 6 comments
Open

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

pavpanchekha opened this issue Oct 6, 2024 · 6 comments
Labels
bug Something isn't working css has repro We have a way to reproduce this bug. layout reduction of web content Issue has a simplified reduction based on real-world web content. web compatibility

Comments

@pavpanchekha
Copy link
Contributor

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:

image

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:

image

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

image
@pavpanchekha
Copy link
Contributor Author

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

@pavpanchekha
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

pavpanchekha commented Oct 6, 2024 •
edited
Loading

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
Copy link
Contributor Author

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).

@AtkinsSJ AtkinsSJ added bug Something isn't working reduction of web content Issue has a simplified reduction based on real-world web content. has repro We have a way to reproduce this bug. web compatibility css layout labels Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working css has repro We have a way to reproduce this bug. layout reduction of web content Issue has a simplified reduction based on real-world web content. web compatibility
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@pavpanchekha @AtkinsSJ