1684236 - Implement align-content for block containers

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, enhancement)

Description

[fantasai]

Attachment: basic dynamic testcase (no fragmentation)

CSS Box Alignment defines align-content to apply to blocks: https://www.w3.org/TR/css-align-3/#distribution-block

This is a separable part of https://bugzilla.mozilla.org/show_bug.cgi?id=1105571

Comment 1

[fantasai]

Attachment: wip

This renders the basic testcase correctly. It doesn't handle fragmentation, or weird dynamic mixes of content-based and fixed min/max/preferred sizes (ComputeFinalSize still needs adjustment to its content-based size calculations).

What I'm stuck on: There's basically two ways to deal with reflow: either reset the alignment shift to zero every time we do a reflow pass and then re-adjust in AlignContent() later, or cache the shift and assume most things won't move, and re-adjust when they do. This patch takes the latter approach, but to get that right in the fragmentation cases requires adjusting the availableBSize--and unfortunately availableBSize is const.

Fwiw, I noticed that we're already hacking around this immutability in https://searchfox.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp#1279 with this "mutableReflowInput" stuff. If that's the recommended method of dealing with this problem, I can try to copy it (but I can't say I really understand what it's doing). But it doesn't seem great?

Anyway, the options afaict are to

• go with the zero-reset-on-reflow approach: most reflows will end up positioning the frames twice
• do something about adjusting availableBSize to allow the code to fragment correctly; reposition frames only if needed
• tolerate the mismatch in applying the shift to mBCoord vs availableBSize and kick off a second ReflowDirtyLines pass to pull or push affected lines in the cases where we detect a problem

Thoughts?

Comment 2

[fantasai]

Attachment: wip2

[Patched up ComputeFinalSize, shifted some stuff into BlockReflowInput; comments/questions above still apply, just wanted to post the latest]

Comment 3

[Mats Palmgren (inactive)]

(In reply to fantasai from comment #1)

Anyway, the options afaict are to

• go with the zero-reset-on-reflow approach: most reflows will end up positioning the frames twice
• do something about adjusting availableBSize to allow the code to fragment correctly; reposition frames only if needed
• tolerate the mismatch in applying the shift to mBCoord vs availableBSize and kick off a second ReflowDirtyLines pass to pull or push affected lines in the cases where we detect a problem

Thoughts?

Applying a stored alignment offset from a previous reflow and letting that affect fragmentation seems like it would lead to unstable layout.
Imagine you have a 100px block with align-content:end and it has a child that is 80px. Now flow that in a 200px fragmentainer, which gives you a non-fragmented result and you store 20px as an offset; then let's say the fragmentainer is resized to 50px and we reflow it again. Now we apply the alignment offset and fragmenting the block's children with 30px available size.
Compare that to reflowing the fragmentainer at 50px directly, with no stored offset on the block. Now you fragment the content with 50px available size. After the reflowing the first fragment of the block we have no clue what size its content is going to be so we calculate a correct alignment offset at this point. Furthermore, fragmenting the block's content at different places may lead to it having different sizes because margins between children for example are not applied at fragment boundaries. So I think it would lead to unstable results to tentatively apply an offset and then try to "adjust and reflow lines again".

So, for the fragmentation case, I think the only reasonable approach is to never store an offset and always start at zero. If the block is FULLY_COMPLETE(*), then you can calculate an offset and apply it; otherwise, just don't apply the alignment at all (i.e. start-alignment). It also makes no sense to me to align individual fragments independently, since non-start alignment will likely lead to strange gaps.

(*)Maybe we can handle OVERFLOW_INCOMPLETE the same way too, but that requires reflowing the lines again to get the overflowing descendants to have the correct fragmentation/size.

For unconstrained reflow we can always calculate the correct offset and apply it, so we could store it as an optimization. I'm not convinced it's really worth it though, so I would suggest we skip it for the first implementation and then collect some metrics on that first. Also, I suspect the inline-size of orthogonal writing-mode children could be affected by applying an offset up-front, which worries me that it might lead to unstable layout results similar to the fragmentation case.

So for now, I would recommend we do the simplest possible implementation:

1. never store an offset
2. if the reflow was INCOMPLETE, or the frame is a continuation, then don't apply any alignment at all

Comment 4

[Mats Palmgren (inactive)]

I see that you basically handle the fragmentation case as I suggested here:

      || IsTrueOverflowContainer() // nothing to shift
      || GetPrevInFlow() || !aState.mReflowStatus.IsFullyComplete()) {
    return;

FYI, I think the IsTrueOverflowContainer() is redundant since GetPrevInFlow() is always true for those.

Comment 5

[Mats Palmgren (inactive)]

+  if (StylePosition()->mAlignContent.primary != mozilla::StyleAlignFlags::NORMAL) {
+    AlignContent(state, aMetrics, &blockEndEdgeOfChildren);

minor nit: we could just handle NORMAL in the early return in AlignContent with the other style value checks there.

+  if (!(alignment == mozilla::StyleAlignFlags::CENTER ||
+        alignment == mozilla::StyleAlignFlags::SPACE_AROUND ||

SPACE_EVENLY also falls back to CENTER. Maybe just add a local bool isCenterAligned = ... and then use that in this condition and in the later check for those values?

+        alignment == mozilla::StyleAlignFlags::LAST_BASELINE) // no shift necessary

I don't see any support in the spec for making LAST_BASELINE behave as END by default.
The spec says:

If a <content-distribution> is specified its fallback alignment is used instead.

I can't find any spec text to suggest that <baseline-position> also should unconditionally use fallback alignment for blocks.
On the contrary, Baseline Content-Alignment says that the container should align the subjects by adjusting their padding if they participate in content-alignment. This is something the grid/flex container does on the items once it knows the baseline offsets of all the items in the group. Only if some condition for participating is false should we use the fallback (per the "Otherwise, ..." paragraph at the end). Note that there is no requirement that a baseline group must contain more than one item for participation, so a grid with just one last baseline-aligned item still don't use the fallback. E.g.:

<div style="display:grid; height:100px; border:solid">
  <div style="height:70px; align-content:last baseline; background:grey">at top of item?</div>
</div>

Currently, this renders the text at the top of the item, which I believe is correct, whereas the suggested patch would render it at the bottom.
(Chrome renders this wrong since align-self:stretch should fall back to self-end in this case and thus the item should be aligned at the end.)

This on the other hand should take the fallback and render the text at the bottom of the item, as your patch does:

<div style="display:grid; height:100px; border:solid">
  <div style="height:70px; align-content:last baseline; background:grey; align-self:start">at bottom of item?</div>
</div>

since the item doesn't participate in baseline content-alignment in this case. (Currently, UAs renders this case wrong b/c they don't support end on block containers.)

So, I think we should probably add a bit on the ReflowInput to signal this case ("this frame participates in baseline content-alignment") that this code can look at, in addition to LAST_BASELINE. Then the flex/grid/table-row container can set that bit to true to inhibit this fallback end-alignment as needed. (the bit would always be false if the container isn't a flex/grid/table-row container)
WDYT?

Comment 6

[Mats Palmgren (inactive)]

+  if (!(StylePosition()->mAlignContent.primary & mozilla::StyleAlignFlags::UNSAFE)) {
+    shift = std::max(0, shift);

This handles an unspecified overflow-position as safe. I think we should handle it as unsafe until we implement the "smart" behavior. That's how it's currently handled in a flex/grid container.

Comment 7

[fantasai]

Also, I suspect the inline-size of orthogonal writing-mode children could be affected by applying an offset up-front, which worries me that it might lead to unstable layout results similar to the fragmentation case.

Hm. Can you explain a bit more about this case? I don't think orthogonal writing-mode children should be affected by an offset, they're supposed to size based off the container size, not consider their offset from the content edge.

... Now we apply the alignment offset and fragmenting the block's children with 30px available size.

This is precisely the type of problem I'm trying to avoid with the approaches in the original question. :)

So, for the fragmentation case, I think the only reasonable approach is to never store an offset and always start at zero.

We could do that, but we'd have to do it unconditionally in any fragmentation context. This condition: “if the reflow was INCOMPLETE, or the frame is a continuation, then don't apply any alignment at all” is insufficient for avoiding trouble. Consider a box that is fully complete, but overflowing and unsafe-centered, and the fragmentainer is later reflowed at a shorter height... it will try to fragment the overflow but we won't know that until after we apply the cached shift. (It would have fragmented the overflow anyway in a clean reflow, but at a different point.)

Let me take a shot at getting overflow to fragment correctly. I think I can do it, the main issue is doing it cleanly.

FYI, I think the IsTrueOverflowContainer() is redundant since GetPrevInFlow() is always true for those.

Good point. :) I'll make a note to clean that up after I deal with fragmentation.

I don't see any support in the spec for making LAST_BASELINE behave as END by default.

Spec: “The fallback alignment for last baseline is safe self-end (for self-alignment) or safe end (for content-distribution).”

I can't find any spec text to suggest that <baseline-position> also should unconditionally use fallback alignment for blocks.

Spec: “If a box does not belong to a shared alignment context, then the fallback alignment is used. ... The fallback alignment is also used to align the baseline-sharing group within its alignment container.” Example for the latter case: two items last-baseline aligned in a grid row that is taller than either item, should be aligned to each other and then pushed endward as far as possible otherwise.

From an authoring perspective, 'last baseline' is like a more refined version of end alignment. So if it's not actually aligning with anything, it should fall back to end. And if it is aligning with other last-baseline things, then they should all be pushed endward insofar as possible while maintaining that alignment. If there's anything in the spec to the contrary, we should probably fix that in the spec...

Currently, this renders the text at the top of the item, which I believe is correct, whereas the suggested patch would render it at the bottom.
(Chrome renders this wrong since align-self:stretch should fall back to self-end in this case and thus the item should be aligned at the end.)

Why does it fall back to self-end in this case? I mean, that's certainly a worthy idea that I would support, but I don't remember speccing it. :)

SPACE_EVENLY also falls back to CENTER. Maybe just add a local bool isCenterAligned = ... and then use that in this condition and in the later check for those values?

Done.

minor nit: we could just handle NORMAL in the early return in AlignContent with the other style value checks there.

Fair. I keep forgetting that compilers can probably optimize these things themselves these days. :)

This handles an unspecified overflow-position as safe. I think we should handle it as unsafe until we implement the "smart" behavior. That's how it's currently handled in a flex/grid container.

I thought about this and, given that we do have some amount of compat risk here, I was thinking it's best to go with safe alignment for blocks at least initially? Can bring that up with the WG also, and see what they think. It's an easy change either way.

(I'll add a historical note also: we didn't add the safe/unsafe keywords until later and in flexbox at least the idea was that margins could be used for safe alignment so we made the alignment properties unsafe. Probably if we knew about the safe/unsafe keywords at that time, we would have been better off making it safe, since frequently authors aren't paying attention to failure cases. None of that applies to block layout.)

It also makes no sense to me to align individual fragments independently, since non-start alignment will likely lead to strange gaps.

I thought about this, and I think it does make sense to align individual fragments independently in many cases... but because of the way blocks are multi-levelled, it's not so simple. (We want to preserve the invariant that an intervening unstyled block has no effect on layout, ideally.)

Comment 8

[Mats Palmgren (inactive)]

Attachment: available size test

(In reply to fantasai from comment #7)

Hm. Can you explain a bit more about this case? I don't think orthogonal writing-mode children should be affected by an offset, they're supposed to size based off the container size, not consider their offset from the content edge.

This testcase illustrates that we take a constrained available size into account.
Chrome doesn't seem to do that though, so hmm, maybe just a bug?

Comment 9

[fantasai]

I can definitely see why it makes sense to do that in some cases, but as the remaining space shrinks it'll end up clipping content off the side of the page; better to push to a fresh page where the full height of the ICB is available. So yeah, just a bug. :) https://www.w3.org/TR/css-writing-modes-3/#orthogonal-layout (note the “available space” there is different from the “available space” in Gecko)

Comment 10

[fantasai]

Attachment: overflow fragmentation dynamic testcase (end alignment)

Comment 11

[fantasai]

Attachment: wip3

Comment 12

[fantasai]

Alright, this one handles fragmentation correctly. It disables alignment when the content being aligned is fragmented, and limits the amount of shift to what would not trigger any fragmentation. Like the previous patch, it's optimized for maintaining the previous alignment shift rather than resetting to zero and re-aligning on every reflow.

What's not handled correctly right now is switching between normal and non-normal alignment, since this should switch a normal block between flow vs flow-root. IIRC this requires reconstructing the frame? If that's still true, then I'm unsure how to handle it in nsStylePosition::CalcDifference. Ideally, we don't want to reconstruct the frame unless it's a block frame, but nsStylePosition doesn't have access to nsStyleDisplay afaict? If I've got that right, then the two options we have are to a) reconstruct the frame on normal <-> non-normal changes regardless of frame type or b) move align-content into nsStyleDisplay so that it can make frame reconstruction conditional on block display. Thoughts?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to fantasai from comment #12)

What's not handled correctly right now is switching between normal and non-normal alignment, since this should switch a normal block between flow vs flow-root. IIRC this requires reconstructing the frame? If that's still true, then I'm unsure how to handle it in nsStylePosition::CalcDifference. Ideally, we don't want to reconstruct the frame unless it's a block frame, but nsStylePosition doesn't have access to nsStyleDisplay afaict? If I've got that right, then the two options we have are to a) reconstruct the frame on normal <-> non-normal changes regardless of frame type or b) move align-content into nsStyleDisplay so that it can make frame reconstruction conditional on block display. Thoughts?

That's definitely solvable. We have some difference bits methods that look at various structs here, or you could use DO_STYLE_DIFFERENCE_WITH_ARGS to pass nsStyleDisplay down to nsStylePosition::CalcDifference.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 15

[fantasai]

Attachment: wip4 (from March 2021)

Archiving the last valid state of this fix, which IIRC addressed the remaining comments and just needed tests...

Needs some work before it'll apply cleanly to current mozilla-central, though. Let's see if I can get it to run while I still can...

Comment 16

[fantasai]

Attachment: wip5

OK, this builds, and also handles fragmentation.

Limitations:

1. It does not fragment overflowing content in order to align the content; it bails out of alignment entirely in this case.
2. It can be stateful if you trigger cases that would require it, and then don't (but correctly handles cases that didn't, and then do) because reflow isn't triggered in these cases.

Still needs WPT tests, though.

Comment 17

[fantasai]

Attachment: regular fragmentation dynamic testcase (end alignment)

Comment 18

[fantasai]

Attachment: wip6

• fixes float handling
• minor clean-up
• adds comprehensive WPT tests for continuous media

Still need fragmentation reftests...

Comment 19

[fantasai]

Attachment: wip7

• fixes statefulness in the fragmentation-with-overflow case
• fully disables alignment when the box is incomplete, since getting that to work per-fragment with nested content is complicated (should be layered on top of this)
• still needs fragmentation testcases :) (it passes the tests, but they're not automated)
  Pushed to tryserver, waiting to see if it breaks anything...

Comment 20

[fantasai]

Tryserver reports 4 test failures that don't seem relevant? https://treeherder.mozilla.org/push-health/push?repo=try&revision=92b58665283e0ec1e7626315db637ed2a712b788

Comment 21

[fantasai]

Attachment: Bug 1684236 - patch 1 - cleaner casting for mutable aReflowInput in nsBlockFrame::Reflow

Keeps us with one source of truth for reflowInput.

Comment 22

[fantasai]

Attachment: Bug 1684236 - Implement 'align-content' on block containers. r=jfkthame,#layout

Depends on D181857

Comment 23

[fantasai]

Attachment: fragmentation follow-up WIP

Attaching this here as follow-up for later. It contains

• rough draft of limiting the alignment when printing so that we don't lose data even for unsafe alignment
  -> This will be necessary if we ever make this the default for blocks.
  -> Similar logic would be useful for grid/flex also.
  -> It needs to be rewritten into a post-reflow callback because we don't have our ancestors’ positions finalized during reflow :(
• notes on how to make this work for alignment within fragments; just switching up the conditional does work (I tested it), except when there's a fragmenting child. (Since that's inconsistent, I opted to just disable alignment when the alignment container fragments here.)

Comment 24

[fantasai]

Attachment: Bug 1684236 - patch 3 - Add basic static reftests for align-content fragmentation

Depends on D181858

Comment 25

[fantasai]

OK, canadahonk helped me actually submit this for review properly (I think). :)

I also added a separate commit for adding basic fragmentation tests. They're not as rigorous on margins as they should be because we have bugs about margins at breaks on BFC roots... so I can't write a correct reference for margin behavior and have the test pass. :/

Comment 26

[fantasai]

And here's an explanation of the patch, which should hopefully help with reviewing it:
https://fantasai.inkedblade.net/weblog/2023/align-content/

Comment 27

[Emilio Cobos Álvarez (:emilio)]

ni? to:

• Land the test.
• Fix up the first patch so that it doesn't leave the available size set on the caller, or so that ReflowInput is mutable.
• Fix up the second patch to add a pref.

Comment 28

[Phabricator Automation]

Comment on attachment 9340651 [details]
Bug 1684236 - patch 1 - cleaner casting for mutable aReflowInput in nsBlockFrame::Reflow

Revision D181857 was moved to bug 1854634. Setting attachment 9340651 [details] to obsolete.

Comment 29

[Emilio Cobos Álvarez (:emilio)]

Attachment: WIP: Bug 1684236 - Untested pagination changes from fantasai with a couple fixes.

Posting for reference since:

• This needs tests.

• display-list based fragmentation should actually make most of these
  work even if alignment causes overflow.

• It's unclear to me if the extra "overflow reflow" stuff would be
  needed, we don't generally need to care about dynamic reflow in
  paginated contexts.

In any case we should probably not land this without tests.

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1684236 - Fix some test reference links. r=jfkthame,#layout

These tests are the only ones related to this feature that fail, and they just
seem to have the wrong reference.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0b878dae244
Implement 'align-content' on block containers. r=layout-reviewers,jfkthame

Comment 33

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b0b878dae244

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Is this something we should call out in the Fx125 relnotes?

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46a010c61a93
Fix some test reference links. r=jfkthame,layout-reviewers

Comment 36

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/44858 for changes under testing/web-platform/tests

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7237bf9155b
Annotate a non-fatally-asserting test.

Comment 38

[Emilio Cobos Álvarez (:emilio)]

Release Note Request (optional, but appreciated)
[Why is this notable]: Trivial vertical centering in block layout
[Affects Firefox for Android]: yes
[Suggested wording]: align-content works now in block layout, allowing block direction alignment without needing a flex or grid container
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_box_alignment/Box_alignment_in_block_abspos_tables and https://developer.chrome.com/blog/align-content look nice.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Added to the Fx125 relnotes.

Comment 40

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/46a010c61a93
https://hg.mozilla.org/mozilla-central/rev/b7237bf9155b

Comment 41

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 42

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot