1105571 - [meta] Support CSS Box Alignment properties on blocks

Status: NEW

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We currently support the properties in CSS Box Alignment http://dev.w3.org/csswg/css-align/ on flexbox, but I don't believe we support them on blocks.  We should.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, this is important to do sooner rather than later, because the longer we wait, the greater the risk that Web compatibility will require that we not do this.  However, it addresses some common developer requests like vertical centering, so it seems worth doing.

Comment 2

[Brad Werth [:bradwerth]]

Attachment: Bug 1105571 Part 3: Add a css Box Align test to ensure block containers with box align styles respond similarly to flex containers.

Review commit: https://reviewboard.mozilla.org/r/100300/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100300/

Comment 3

[Brad Werth [:bradwerth]]

Attachment: Bug 1105571 Part 1: Change ConstructScrollableBlock and ConstructNonScrollableBlock to construct containers instead of blocks.

Review commit: https://reviewboard.mozilla.org/r/100302/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100302/

Comment 4

[yisibl]

It will be very interesting

Comment 5

[Brad Werth [:bradwerth]]

Comment on attachment 8820915 [details]
Bug 1105571 Part 3: Add a css Box Align test to ensure block containers with box align styles respond similarly to flex containers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100300/diff/1-2/

Comment 6

[Brad Werth [:bradwerth]]

Comment on attachment 8820916 [details]
Bug 1105571 Part 1: Change ConstructScrollableBlock and ConstructNonScrollableBlock to construct containers instead of blocks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100302/diff/1-2/

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1105571 Part 2: Add a static selector to swap flex containers in for block containers for non-body blocks.

Review commit: https://reviewboard.mozilla.org/r/103408/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103408/

Comment 8

[Brad Werth [:bradwerth]]

Comment on attachment 8820916 [details]
Bug 1105571 Part 1: Change ConstructScrollableBlock and ConstructNonScrollableBlock to construct containers instead of blocks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100302/diff/2-3/

Comment 9

[Brad Werth [:bradwerth]]

Comment on attachment 8820915 [details]
Bug 1105571 Part 3: Add a css Box Align test to ensure block containers with box align styles respond similarly to flex containers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100300/diff/2-3/

Comment 10

[Brad Werth [:bradwerth]]

Comment on attachment 8820916 [details]
Bug 1105571 Part 1: Change ConstructScrollableBlock and ConstructNonScrollableBlock to construct containers instead of blocks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100302/diff/3-4/

Comment 11

[Brad Werth [:bradwerth]]

Comment on attachment 8825228 [details]
Bug 1105571 Part 2: Add a static selector to swap flex containers in for block containers for non-body blocks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103408/diff/1-2/

Comment 12

[Brad Werth [:bradwerth]]

Comment on attachment 8820916 [details]
Bug 1105571 Part 1: Change ConstructScrollableBlock and ConstructNonScrollableBlock to construct containers instead of blocks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100302/diff/4-5/

Comment 13

[Brad Werth [:bradwerth]]

Comment on attachment 8825228 [details]
Bug 1105571 Part 2: Add a static selector to swap flex containers in for block containers for non-body blocks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103408/diff/2-3/

Comment 14

[Brad Werth [:bradwerth]]

Comment on attachment 8820915 [details]
Bug 1105571 Part 3: Add a css Box Align test to ensure block containers with box align styles respond similarly to flex containers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100300/diff/3-4/

Comment 15

[Brad Werth [:bradwerth]]

Comment on attachment 8820915 [details]
Bug 1105571 Part 3: Add a css Box Align test to ensure block containers with box align styles respond similarly to flex containers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100300/diff/4-5/

Comment 16

[Brad Werth [:bradwerth]]

Comment on attachment 8825228 [details]
Bug 1105571 Part 2: Add a static selector to swap flex containers in for block containers for non-body blocks.

dholbert, would you please evaluate if this patch is adequate as a web compatability test of actually implementing the box align properties for block containers?

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8825228 [details]
Bug 1105571 Part 2: Add a static selector to swap flex containers in for block containers for non-body blocks.

https://reviewboard.mozilla.org/r/103408/#review124688

::: layout/base/nsCSSFrameConstructor.cpp:3027
(Diff revision 3)
> +    uint16_t childJS = styleContext->StylePosition()->mJustifySelf;
> +    uint16_t childAS = styleContext->StylePosition()->mAlignSelf;

(I suspect we don't need to care about align-self on child frames... child frames are "block-level boxes", and https://drafts.csswg.org/css-align/#propdef-align-self says "the align-self property does not apply to block-level boxes" -- by which I *think* the spec means "any boxes participating in a block formatting context")

::: layout/base/nsCSSFrameConstructor.cpp:5026
(Diff revision 3)
> +  ContainerFrameCreationFunc constructor =
> +    ShouldRemapBlockToFlex(aState, aItem) ?
> +    NS_NewFlexContainerFrame :
> +    NS_NewBlockFrame;

So it looks like the goal here is to create a flex container instead of a block container, in cases where we have some non-default CSS-Align properties, so that we can let the flex container handle css-align properties and simulate how things will look once we have actual support in blocks.

However, I don't think the "simulation" that this provides will be particularly useful.  The whole idea would be to look for broken layout, but this simulation is likely to break the layout significantly more than necessary, due to unrelated block/flex differences (and due to properties that behave differently in block vs. flex).

I would suggest that instead, you add some telemetry & debug-only logging -- perhaps inside of ConstructNonScrollableBlock or nsBlockFrame::Init -- to detect this scenario and report about it.  Then we can actually ship that patch (and maybe even uplift to aurora) to get some metrics about how often the potentially-problematic styles that dbaron is worried about actually occur in the wild.

I'm not sure how best to structure the telemetry -- perhaps as a single-bit flag on the document or on the PresContext to indicate "we had some block on this page that has a non-default value of a css-align property"...?  That would give us an idea of how many pages might be affected, I think.

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #17)
> (I suspect we don't need to care about align-self on child frames... child
> frames are "block-level boxes", and
> https://drafts.csswg.org/css-align/#propdef-align-self says "the align-self
> property does not apply to block-level boxes" -- by which I *think* the spec
> means "any boxes participating in a block formatting context")

Ah -- one minor terminology tangent: I sorted out the thing I was slightly confused about here (my "I *think* the spec means...).  I was initially thrown by how css-align exempts block-level boxes in this specific case (for align-self) but doesn't seem to exempt inline-level boxes.  So I thought maybe the spec editors intended to hand-wavily lump the two together via a slight abuse of the "block-level" term.  But actually, the spec does exempt inline-level boxes from *all* of these properties much earlier, in the abstract & introduction: "The alignment of text and inline-level content is defined in [another spec]").

So anyway - bottom line, it indeed seems like "align-self" has no layout effect on *any* children of a block container (whether those children are inline-level or block-level), so we shouldn't care about it when screening for possible-breakage here.

> ::: layout/base/nsCSSFrameConstructor.cpp:5026
> I would suggest that instead, you add some telemetry & debug-only logging --
> perhaps inside of ConstructNonScrollableBlock or nsBlockFrame::Init -- to
> detect this scenario and report about it.

(Wherever we add the logging/telemetry, I suppose it needs to be a place where we know about the block's children, so that we can screen them (the block-level ones at least) for "justify-self".  So maybe we should put this logging/telemetry in nsBlockFrame::Reflow or one of its helpers.  Reflow may get called many times, of course, which means we probably *do* want to make this a single on/off switch for the document ["has this ever happened in this document"] rather than a raw count of how many times it happened.)

Also, this hypothetical telemetry/logging should probably happen in a helper-bug rather than in this bug here, so that your & my further thoughts on this preliminary testing don't distract from the main thrust of this bug, which is about the *actual* css-align implementation for blocks. :)

Comment 19

[Mats Palmgren (inactive)]

The spec for 'justify-self' for blocks seems odd to me. At the end of 6.1
it says "Values other than 'stretch' cause a width/height of auto to be
treated as fit-content." which is fine, until I saw that 'normal' behaves
as 'start' in 6.1.1:
https://drafts.csswg.org/css-align-3/#justify-block

Shouldn't 'normal' behave as 'stretch' for blocks? (and block-level
flex/grid containers too for that matter)
Am I missing something?

Comment 20

[Tab Atkins Jr.]

Mats: Yeah, you're right; it looks like fantasai and I accidentally messed that one up. The behavior of "normal" on blocks has been changed to "stretch" now, so it won't trigger fit-content sizing.

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Brad, what's needed to drive this forward?  I see that telemetry was added and then removed.  Did we learn that it is safe to proceed here?

Comment 22

[Brad Werth [:bradwerth]]

We didn't learn anything useful because I didn't adequately test the final version of the telemetry probe; it was overbroad and reported 100% false positives. It was a learning moment for me.

The current thinking that I've heard is that since there is intent-to-implement from other vendors (possibly apocryphal -- I don't have the working group notes), we should proceed with implementation and put the behavior behind a preference while we do real-world testing. The implementation was not trivial for me when I looked at it months ago, but I'm willing to take another run at it.

Comment 23

[Jen Simmons [:jensimmons]]

Yes, my understanding from attending CSSWG meetings is that browsers across the field (the major browsers) have all agreed to implement. There is desire to do so sooner rather than later in order to prevent web compat problems. I haven't heard anyone else express a sense of data, of fear or of confidence. 

I'd love for us to get going and to try. Having this feature on the web will be incredibly useful to developers.

Comment 24

[Daniel Holbert [:dholbert]]

Note: when we [or probably any other browser] implement this, we'll likely need to adjust one of the pieces of our "Inactive CSS" DevTools feature. (metabug for that feature: bug 1540753)

At this point, the "Inactive CSS" devtools feature will probably have a piece that shows a warning when align/justify properties are used in a non-flex/grid context, and it'll tell the author that these properties have no effect there, in current browser implementations. For now, that is accurate, but once we fix this bug, it'll no longer be accurate.

(For now, though, this warning will [usefully] reduce the likelihood that these properties will creep around into inactive spots in author's stylesheets, in places where they currently have no effect but will suddenly start causing breakage when we land a fix here.)

Comment 25

[Sean Voisen (:svoisen)]

Brad: Is it fair to say you are no longer working on this right now?

Comment 26

[Brad Werth [:bradwerth]]

(In reply to Sean Voisen (:svoisen) from comment #25)

Brad: Is it fair to say you are no longer working on this right now?

Yes, I haven't looked at this in a long time. I hope somebody can move it forward.

Comment 27

[Sebastian Zartner [:sebo]]

Removing the dev-doc-needed flag from this meta-issue, as support is obviously added incrementally. So the flag should be set on the individual blocking bugs.

Sebastian