Open Bug 1105571 Opened 10 years ago Updated 2 years ago

support CSS Box Alignment properties on blocks

Categories

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

Product:
Core
Component:
Layout: Block and Inline
Platform:
x86_64
Linux
Type:
enhancement

Tracking

()

People

(Reporter: dbaron, Unassigned)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed, DevAdvocacy, Whiteboard: [DevRel:P1] [layout:backlog])

Attachments

(3 obsolete files) 

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.
Assignee: nobody → bwerth
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.
Keywords: DevAdvocacy
Whiteboard: [DevRel:P1]
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-
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?
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.
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?
Flags: needinfo?(bwerth)
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.
Flags: needinfo?(bwerth)
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.
Status: NEW → ASSIGNED
Blocks: 1492055

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.)

Type: defect → enhancement
Whiteboard: [DevRel:P1] → [DevRel:P1] [layout:backlog]

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

Flags: needinfo?(bwerth)

(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.

Assignee: bwerth → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(bwerth)
Attachment #8820916 - Attachment is obsolete: true
Attachment #8825228 - Attachment is obsolete: true
Attachment #8820915 - Attachment is obsolete: true
Depends on: 1684236
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: