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

Layout 2020: Float clearance can be negative #29904

Open
Loirooriol opened this issue Jun 21, 2023 · 6 comments · Fixed by #29946
Open

Layout 2020: Float clearance can be negative #29904

Loirooriol opened this issue Jun 21, 2023 · 6 comments · Fixed by #29946
Labels
A-layout/floats A-layout/2020 https://github.com/servo/servo/wiki/Layout-2020

Comments

@Loirooriol
Copy link
Contributor

https://drafts.csswg.org/css2/#clearance says

Note: The clearance can be negative or zero

I need to check the spec in detail, but I suspect that this is one of these cases:

<!DOCTYPE html>
text
<div style="outline: solid; width: 100px">
  <div style="float: left; width: 25px; height: 25px; background: cyan"></div>
  <div style="clear: both; height: 50px; margin-top: 100px; background: magenta"></div>
</div>

All Firefox, Blink and WebKit do something like this:

But the margin separates the cyan and magenta boxes in both layout 2013 and 2020.

Also, not sure if having a clearance of zero might be different than having no clearance in some corner cases.
@Loirooriol Loirooriol added A-layout/floats A-layout/2020 https://github.com/servo/servo/wiki/Layout-2020 labels Jun 21, 2023
@mrobinson mrobinson changed the title Clearance can be negative Layout 2020: Float clearance can be negative Jun 21, 2023
@Loirooriol
Copy link
Contributor Author

This is my understanding of the spec. First we consider the hypothetical position as if clear was none:

This hypothetical position of the element’s top border edge is not past the relevant floats, so clearance is introduced, and margins collapse according to the rules in 8.3.1. That is, the margin no longer collapses with the start margin of the parent because they are no longer adjoining since they are separated by clearance.

So now we are working with something like this (but the magenta element will be moved by the clearance):

image

The spec allows 2 options:

  • Option 1: Clearance is set exactly to the amount necessary to place the border edge of the block even with the bottom outer edge of the lowest float that is to be cleared. That is, -100px.

  • Option 2: Clearance is set to the greater of:

    • The amount necessary to place the border edge of the block even with the bottom outer edge of the lowest float that is to be cleared. That is, the same as the option 1, -100px.
    • The amount necessary to place the top border edge of the block at its hypothetical position. It's already at the hypothetical position (100px below the float), so 0px

It seems that, at least in this case, browsers use option 1, resulting in a clearance of -100px.

Servo is doing something that looks closer to option 2, but it's wrong because:

Note the spec says:

Both behaviors are allowed pending evaluation of their compatibility with existing Web content. A future CSS specification will require either one or the other.

If browsers are using option 1 then that's what a future CSS Flow spec will choose, so we should probably align with them.

@Loirooriol Loirooriol linked a pull request Jun 29, 2023 that will close this issue
Layout 2020: implement clearance as Option<Length> #29946
Merged
3 tasks
@Loirooriol Loirooriol mentioned this issue Jun 29, 2023
Merged
4 tasks
@Loirooriol
Copy link
Contributor Author

Loirooriol commented Jul 1, 2023 •
edited

So, the problem is that when calculate_clearance computes the hypothetical block position, it doesn't take into account that the margin can collapse with the parent and then move previous floats down, requiring clearance.

I have checked other browsers. Blink always inserts clearance when we would otherwise collapse with the parent, and this could move a relevant float. Firefox and WebKit only do so when the bottom outer edge of the float would overlap the element with clear.

Firefox and WebKit may seem to make more sense, but it seems more tricky so I guess aligning with Blink would be good enough.

@joshua-holmes
Copy link
Contributor

I definitely want to work on this, but I want to look at it a little bit more to come up with a concrete plan before making any changes

@joshua-holmes
Copy link
Contributor

Servo does not seem to be behaving the same way that it did when the ticket was created. It is now behaving as if clear is set to none. (see photo)
pic

Regardless, this can be fixed.

@Loirooriol , I'm not sure I understand your previous comment, especially regarding "collapsing". It seems to me like the margin should be "collapsing" when clear is applied, not the other way around.

However, I do understand the comment before that. I think that option 1 is the simplest and probably easiest to maintain.

Another option that I've considered is simply ignoring the margin on the element with clear, however, I don't like that approach much. I prefer to use the value of the bottom of the relevant float element to place the clear element

@Loirooriol
Copy link
Contributor Author

@joshua-holmes No, applying clear can potentially introduce clearance. Margins are not adjoining when separated by clearance, so they can't collapse.

However, I don't recommend working on this issue as an introduction to Servo. Doing it properly requires doing speculative layouts, and undoing if the assumptions were wrong. I discussed some approaches with Ian Kilpatrick, who did a similar work in Blink, and he said that these details of block layout are the most tricky to implement in layout, and that things like flex and grid are much more straightforward.

So maybe you would be interested in some flexbox work instead? There is a partial implementation, but lots of things missing or needing refinement.

@joshua-holmes
Copy link
Contributor

@Loirooriol , thanks for the insight! I didn't realize I picked such a complex task. I agree, I shouldn't pick something complex until I have some experience in the system. I'll take your advice and look into one of the recommended topics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-layout/floats A-layout/2020 https://github.com/servo/servo/wiki/Layout-2020
Projects
Layout 2020 Floats
Status: Todo
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Layout 2020: implement clearance as Option<Length> Loirooriol/servo
2 participants
@Loirooriol @joshua-holmes