support CSS Box Alignment properties on blocks

NEW
Assigned to

Status

()

Core
Layout: Block and Inline
3 years ago
23 days ago

People

(Reporter: dbaron, Assigned: bradwerth)

Tracking

(Blocks: 1 bug, {dev-doc-needed, DevAdvocacy})

Trunk
x86_64
Linux
dev-doc-needed, DevAdvocacy
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DevRel:P1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
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.
Keywords: dev-doc-needed
(Assignee)

Updated

8 months ago
Assignee: nobody → bwerth
(Reporter)

Comment 1

6 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

5 months ago
It will be very interesting
Keywords: DevAdvocacy
Whiteboard: [DevRel:P1]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

2 months ago
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?
Attachment #8825228 - Flags: feedback?(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.
(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. :)
Attachment #8825228 - Flags: feedback?(dholbert) → feedback-
(Assignee)

Updated

2 months ago
Depends on: 1351383
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?
You need to log in before you can comment on or make changes to this bug.