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

[css-flexbox] Undefined target main size in Resolving Flexible Lengths #5179

Closed
SimonSapin opened this issue Jun 6, 2020 · 8 comments
Closed
Labels
Closed Accepted as Obvious Bugfix Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-flexbox-1 Tested Memory aid - issue has WPT tests

Comments

@SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Jun 6, 2020

A few related questions about and potential bugs in the Resolving Flexible Lengths algorithm https://drafts.csswg.org/css-flexbox/#resolve-flexible-lengths

Background from step 1: here we decide whether to use flex grow factors or flex shrink factors. It’s always one or the other. It’s never neither.

Background from step 2: this step introduces a new concept: a target main size for flex items. This step gives it a value for each inflexible item. For other items however this value is undefined at this point.

Background from step 4.c.: it is marked up as a definition list with four entries like this:

If the remaining free space is zero
Do nothing.
If using the flex grow factor
[…]
If using the flex shrink factor
[…]
Otherwise
Do nothing.

Issue 1: In step 4.c., under “If using the flex grow factor”: this paragraph talks about “the item”, but at this point of the algorithm we are not considering any given item. So, which one? It looks like this paragraph should start with “For every unfrozen item on the line,” like the next one.

Issue 2: The control flow of the algorithm in step 4.c. is not at all clear to me. How do the four entries of the definition list relate to each other? Is each iteration of the loop only supposed to go through one of them? In that case maybe entries 2 and 3 should start with “Otherwise,” to indicate that? If that’s not the case and each entry is independent and to be considered sequentially, then the first entry accomplishes nothing and could be removed. Since we’re always using one of the grow or shrink factor, the fourth entry looks like accomplishes nothing either way and could also be removed. But presumably the editors had a reason to include it, so I feel I’m missing something.

Issue 3: Depending on the resolution of issue 2, the target main size may (if remaining free space is zero) still be undefined for some items by the time we get to step 4.d. of the algorithm, although that step relies on the target main size of all non-frozen items. Similarly, step 5 wants a defined target main size for all items. Should there be a new early step that initializes the target main size of unfrozen items to their flex base size?

@SimonSapin
Copy link
Contributor Author

@SimonSapin SimonSapin commented Jun 6, 2020

Typing all this out made me see what was probably intended. Possible resolutions:

Introduce step zero (before renumbering):

  1. Each item in the flex line has a target main size, initially set to its flex base size. Each item is initially unfrozen and may become frozen. Note: an item’s target main size doesn’t change after freezing.

Restructure step 4.c.:

c. If the remaining free space is non-zero, distribute it proportional to the flex factors:

If using the flex grow factor
For every unfrozen item on the line, […]
Otherwise (Note: using the flex shrink factor),
For every unfrozen item on the line, […]

Bonus drive-by clarification: in step 4.e. “Freeze over-flexed items” add a non-normative note:

This freezes at least one item, ensuring that the loop makes progress and eventually terminates.

@tabatkins
Copy link
Member

@tabatkins tabatkins commented Jun 11, 2020

Issue 1: Yup, it's intended to be the same as the "using the shrink factor" one, so your suggestion is spot on.

Issue 2: Yup, only one gets chosen. The fourth option was indeed a no-op; presumably a leftover from an earlier version? Dunno. Anyway, accepted your suggested edits.

Issue 3: Ah yup, indeed, unfrozen items can skip getting a target main size. Accepted your edits.

@tabatkins tabatkins added Closed Accepted as Obvious Bugfix Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. labels Jun 11, 2020
@SimonSapin
Copy link
Contributor Author

@SimonSapin SimonSapin commented Jun 12, 2020

91bab66 changed the definition of initial free space to use "target main sizes" instead of:

For frozen items, use their outer target main size; for other items, use their outer flex base size.

For the initial free space this is indeed the same, but later on this algorithm is referenced:

Calculate the remaining free space as for initial free space, above.

So this looks like a normative change in the computation of remaining free space, although the commit message says No normative changes intended.

@SimonSapin SimonSapin reopened this Jun 12, 2020
@fantasai fantasai removed Closed Accepted as Obvious Bugfix Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. labels Nov 4, 2020
@andyjakubowski
Copy link

@andyjakubowski andyjakubowski commented May 27, 2021

Issue 3: Depending on the resolution of issue 2, the target main size may (if remaining free space is zero) still be undefined [...]

Thank you for suggesting edits for this. I implemented the Candidate Recommendation version of the algorithm, and encountered the same bug with the possible undefined target main size of an item. I should’ve looked at the Editor’s Draft...

@fantasai
Copy link
Collaborator

@fantasai fantasai commented Jun 9, 2021

@SimonSapin Thanks for catching that. Reverted the change in 88cb91d Let us know if it's correct yet, please? :)

@andyjakubowski Sorry :( :( We're still digging ourselves out of an outdated-publications hole. Out of curiosity, what are you implementing Flexbox into?

@andyjakubowski
Copy link

@andyjakubowski andyjakubowski commented Jun 15, 2021

@fantasai no worries, I caught it when I was testing a prototype 😅

I’m implementing Flexbox into an interactive Flexbox learning course. The idea is that aside from the usual linear narrative structure and an interactive sandbox, you’d be able to peek behind the Flexbox curtain and see how a UA actually lays out a flex box. Kind of like how the Firefox Developer Tools Inspector gives you some insight into the flex base size, min/main size clamping etc. — but on steroids!

I’m still at the prototype stage, but would you like to check it out?

@SimonSapin
Copy link
Contributor Author

@SimonSapin SimonSapin commented Jul 19, 2021

It’s been a year so I have little memory of this, but yes based on reading my own comments it looks like they have all been addressed. Thanks!

@tabatkins
Copy link
Member

@tabatkins tabatkins commented Sep 15, 2021

Tested in web-platform-tests/wpt#30565

@tabatkins tabatkins added the Tested Memory aid - issue has WPT tests label Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed Accepted as Obvious Bugfix Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-flexbox-1 Tested Memory aid - issue has WPT tests
Projects
None yet
Development

No branches or pull requests

4 participants