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

Incorrect height for flex-direction: column inside flex-direction: row #2222

Closed
dhdaines opened this issue Aug 7, 2024 · 8 comments · Fixed by #2231
Closed

Incorrect height for flex-direction: column inside flex-direction: row #2222

dhdaines opened this issue Aug 7, 2024 · 8 comments · Fixed by #2231
Labels
bug Existing features not working as expected
Milestone
63.0

Comments

@dhdaines
Copy link
Contributor

dhdaines commented Aug 7, 2024

Yes, another flexbox bug! I think I have this narrowed down to the specific problem - in the rather common use case of display: flex; flex-direction: column nested inside display: flex; flex-direction: row, the inner elements are not expanded vertically to fit their contents. In other words, this code:

<html>
<head>
<title>Another FlexBox Test</title>
<style>
body {
    max-width: 30em;
    margin: auto;
}
section {
    border: 1px solid blue;
    display: flex;
    flex-direction: row;
}
article {
    border: 1px solid green;
    display: flex;
    flex-direction: column; /* the culprit */
    margin-top: 1ex;
    margin-bottom: 1ex;
}
</style>
</head>
<body>
  <section>
    <article>
      Question 1?  With quite a lot of extra text,
      which should not overflow in the PDF, we hope.
    </article>
    <article>
      Answer 1.  With quite a lot of extra text,
      which should not overflow in the PDF, we hope?
    </article>
  </section>
</body>
</html>

should do this (Chrome):

image

But actually does this (Weasyprint 62.3):

image

(the reason I'm not using grid layout is that actually my CSS is a bit more complex and relies on flex-wrap and flex-basis to make a layout of maximum two columns from a sequence of elements, but all of that works fine, it's just the box sizing that's wrong)
@liZe
Copy link
Member

liZe commented Aug 7, 2024

Thanks for the report. It’s maybe related to #1171.

@liZe liZe added the bug Existing features not working as expected label Aug 7, 2024
@dhdaines
Copy link
Contributor Author

dhdaines commented Aug 7, 2024

Yes it seems like it might be similar (the common denominator is flex-direction: column). I can probably fix it for at least my specific case, not certain about the ones in that bug...

@dhdaines
Copy link
Contributor Author

dhdaines commented Aug 13, 2024 •
edited
Loading

Definitely related to this comment: https://github.com/Kozea/WeasyPrint/blob/main/weasyprint/layout/flex.py#L255

flex-direction:column seems to just be generally broken (which would be the correct interpretation of that comment ;-)), as can be seen by changing the CSS to:

body {
    max-width: 30em;
    margin: auto;
}
section {
    border: 1px solid blue;
    display: flex;
    flex-direction: column;
}
article {
    flex: 1;
    border: 1px solid green;
    margin-top: 1ex;
    margin-bottom: 1ex;
}

Giving this obviously incorrect output:
image

@dhdaines
Copy link
Contributor Author

When flex: 1 (relevant part being flex-grow: 1) is removed from the CSS we get this somewhat better but also incorrect output:
image
So, the basis for children isn't being calculated correctly, and isn't being propagated correctly to the parent.

@dhdaines
Copy link
Contributor Author

Hmm, the last two comments are a different (though vaguely related) issue, it seems. On the browser the available main space is clearly infinite when the main axis is 'height' (i.e. flex-direction is column), but on print media this isn't so clear - is it the remaining space on the page? (Weasyprint thinks so) or is it also infinite (since a flex container could get split across pages)?

If it's the remaining space on the page, then the sizing of <article> above (expanding to fill the page) is correct, it's just the sizing of <section> (not expanding to fit its content) that is wrong.

If it's infinite, then both are wrong, and the reason is that section 9.3.2.D of https://www.w3.org/TR/css-flexbox-1/#algo-main-item is not implemented, as noted here: https://github.com/Kozea/WeasyPrint/blob/main/weasyprint/layout/flex.py#L207 - this is what causes flex-direction: column to not expand flex items on the browser:

Otherwise, if the used flex basis is content or depends on its available space, the available main size is infinite, and the flex item’s inline axis is parallel to the main axis, lay the item out using the rules for a box in an orthogonal flow [CSS3-WRITING-MODES]. The flex base size is the item’s max-content main size.

Do you know what the CSS print media spec says about this?

@dhdaines
Copy link
Contributor Author

So, back to my original problem, the issue here is simply not expanding the flex container in the cross-axis to fit its content. This is likely due to the fact that the code is explicitly setting the height style on the children here: https://github.com/Kozea/WeasyPrint/blob/main/weasyprint/layout/flex.py#L607 which it maybe shouldn't do, as it leads future code to believe that said children have a definite size, which they don't.

@dhdaines
Copy link
Contributor Author

I may be able to make a draft PR this week. I have heavily annotated the flex code already :)

dhdaines added a commit to dhdaines/WeasyPrint that referenced this issue Aug 17, 2024
@dhdaines
fix: use the *used main size* to compute cross size (fixes: Kozea#2222)
87611fb
dhdaines added a commit to dhdaines/WeasyPrint that referenced this issue Aug 17, 2024
@dhdaines
fix: use main size of flex container to grow/shrink (fixes: Kozea#2222)
0aea88f
dhdaines added a commit to dhdaines/WeasyPrint that referenced this issue Aug 17, 2024
@dhdaines
test: test flex-dimension column for issue Kozea#2222
a76fd88
@dhdaines dhdaines mentioned this issue Aug 17, 2024
Merged
@dhdaines
Copy link
Contributor Author

Note that #2231 fixes all the issues above as it turns out that yes, they were all related ;-)

The flexbox layout code is still broken in many ways though!

dhdaines added a commit to dhdaines/WeasyPrint that referenced this issue Aug 17, 2024
@dhdaines
test: test flex-dimension column for issue Kozea#2222
d4f7622
dhdaines added a commit to dhdaines/WeasyPrint that referenced this issue Aug 18, 2024
@dhdaines
fix: use main size of flex container to grow/shrink (fixes: Kozea#2222)
bfeefa4
dhdaines added a commit to dhdaines/WeasyPrint that referenced this issue Aug 18, 2024
@dhdaines
test: test flex-dimension column for issue Kozea#2222
0217f38
@liZe liZe closed this as completed in ce6253f Aug 24, 2024
@liZe liZe added this to the 63.0 milestone Aug 24, 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 Existing features not working as expected
Projects
None yet
Milestone
63.0
Development

Successfully merging a pull request may close this issue.

Fix several problems related to flex-direction: column dhdaines/WeasyPrint
2 participants
@liZe @dhdaines