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.

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

Fix widow pad frames #4209

Closed
wants to merge 2 commits into from
Closed

Fix widow pad frames #4209

wants to merge 2 commits into from

Conversation

MDLC01
Copy link
Contributor

@MDLC01 MDLC01 commented May 21, 2024

Fixes #4234.

It turns out the empty frames created by pad have a non-zero size (they had padding applied to them). This made #2517 not work with those frames.

This PR solves the issue by applying the following transformation: each of the empty frames generated by pad that come before any content become zero-sized.1 In case of a pad element that generates only empty frames, the last one is always kept in order to make sure the element never "disappears".

Footnotes

  1. It is actually a tiny bit more complicated than that, due to some frames having negative dimensions (see this Discord message).

@MDLC01
Copy link
Contributor Author

MDLC01 commented May 21, 2024

Turns out this is harder than I thought. I'm marking this as a draft for now.

@MDLC01 MDLC01 marked this pull request as draft May 21, 2024 11:54
@MDLC01
Copy link
Contributor Author

MDLC01 commented May 21, 2024

Actually, I just forgot to add a test ref! So this PR is now ready for review.

@MDLC01 MDLC01 marked this pull request as ready for review May 21, 2024 16:27
@laurmaedje
Copy link
Member

Is there an associated issue for this?

The whole approach of skipping zero-sized frames in the flow is a bit of hack in my opinion. Adding to this with more complexity in padding seems like a potential maintainance burden to me. It is already very hard to change this in the flow already due to the various ad-hoc bug fixes in it.

@MDLC01
Copy link
Contributor Author

MDLC01 commented May 22, 2024

Is there an associated issue for this?

There is not. I can create one.

Another idea I had after making this PR, which may feel less hacky, was to allow fragments to contain "fillers" instead of frames, which act as zero-sized frames, and are used instead of an empty frame that would have been created to fill a region when nothing fits into it. I don't know how related this would be to #4212 (if at all).

@MDLC01
Copy link
Contributor Author

MDLC01 commented May 23, 2024

I created #4234.

@laurmaedje
Copy link
Member

Let's wait with this until after the layout refactor (if I don't fix it myself during it). Every extra somewhat ad-hoc fix makes incremental refactoring harder.

@MDLC01
Copy link
Contributor Author

MDLC01 commented May 29, 2024

Ok. I'm closing this PR, then.

@MDLC01 MDLC01 closed this May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Element with no content followed by pad can force breakable block to create an empty frame
2 participants