Closed Bug 1269017 Opened 4 years ago Closed 3 years ago

[css-grid] abspos child of a grid should respect "align-self"/"justify-self" when computing its static position

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- affected
firefox52 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(7 files)

STR:
 1. Load attached testcase.

EXPECTED RESULTS:
No red should be visible. (The red element should hide behind the teal element, in the center of the grid.)

ACTUAL RESULTS:
Red element is visible at upper-left.

Relevant spec quote:
# The static position [CSS21] of an absolutely-positioned child
# of a grid container is determined as if it were the sole
# grid item in a grid area whose edges coincide with the
# padding edges of the grid container.
https://drafts.csswg.org/css-grid/#static-position

Given that we have "align-self: center" and "justify-self: center" on the grid, I believe the red abspos child should be centered in the yellow area (" as if it were the sole grid item in a grid area [the size of the padding box]")

The teal element is present just for reference -- it *actually is* the sole grid item in a grid area that's as large as the whole grid, and so it serves as an example for where it the red thing should end up.

Chrome 52 dev edition and Firefox 49 Nightly each give ACTUAL RESULTS.  Maybe neither of us have implemented the current version of this spec section yet?
Summary: [css-grid] abspos child of a grid doesn't respect "align-items"/"justify-items" → [css-grid] abspos child of a grid doesn't respect "align-self"/"justify-self"
Attached file testcase 1
Flags: needinfo?(mats)
It looks like our impl might just be based on an older version of the spec language. In particular, I ran across this quote:
> 385   if (aFlags & AbsPosReflowFlags::eIsGridContainerCB) {
> 386     // When a grid container generates the abs.pos. CB for a *child* then
> 387     // the static-position is the CB origin (i.e. of the grid area rect).
> 388     // https://drafts.csswg.org/css-grid/#static-position
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsAbsoluteContainingBlock.cpp?rev=e74d457b8b11&mark=384-388#384

(I don't see any support for the comment in the spec section that it links to, so I think the spec has changed.)

Note: I'm going to need to get this sort of thing working for flex containers in bug 874718, so I might end up fixing this for grid as well over there.  (Or I might add some flexbox-specific code that we can later enable for Grid, with some tweaks as-needed.)  So, might be good not to dive into this bug here right away, except to confirm that my understanding is correct and that this is indeed a bug.
(Worth noting: in the attached testcase, the grid container is *NOT* the absolute containing block for the abspos thing -- it's simply the parent of the abspos thing (in the DOM tree).  The abspos containing block is the viewport, in this case.

We still need to respect align-self/justify-self when determining the static position of the abspos child, though, per the spec quote in comment 0.)
Depends on: 1269046
I don't think I even considered that the static-position should be
aligned.  I think the spec for it was still being discussed on
www-style at the time I wrote the code here.

Anyway, I'd appreciate if you take this while fixing bug 1269046,
I suspect that the code will be very similar for flexbox/grid.
Flags: needinfo?(mats)
For Grid, adding public methods to align/justify an abs.pos. frame would be
quite easy I think.  The static functions AlignSelf/JustifySelf should work
pretty much as is, except for some minor computed value mappings, e.g.
it currently always maps 'normal' to 'stretch', but for abs.pos. it
should map to 'start' for replaced elements:
https://drafts.csswg.org/css-align-3/#align-abspos

An AbsPosAlignSelf method could map that upfront, calculate the necessary
grid container size, and then call AlignSelf as is I would think.

You could probably generalize these static methods so that they work
for Flexbox too with a few minor tweaks.
s/these static methods/these static functions/
Blocks: 1268999
See bug 1269046 comment 24 for an overview of the abspos + css-align framework that we'll be building on top of in this bug here.

A few additional grid-specific abspos things (which should happen in this bug rather than bug 1269046):

CONTAINING BLOCK FOR ABSPOS CHILD LIST
======================================
(css-grid 10.1, https://drafts.csswg.org/css-grid-1/#abspos-items )
- We'll now need to be finding a grid area (based on "grid-row", "grid-column", etc) for **every frame in a grid's abspos child list** -- and we should be saving *that grid area* in each frame's GridItemContainingBlockRect() proptable entry. (Right now, we always store the grid's content-box rect in that proptable entry, it looks like, per some older spec text IIRC. But that's generally the wrong box.)


ALIGNMENT FOR PLACEHOLDER CHILDREN IN NORMAL CHILD LIST
=======================================================
(css-grid 10.2, https://drafts.csswg.org/css-grid-1/#static-position ) 
- We need to set the flag discussed in bug 1269017 comment 24 (tentatively PLACEHOLDER_STATICPOS_NEEDS_CSSALIGN) on all of the placeholder frames in the grid's normal child list.

- If the grid is *NOT* an abspos CB, we need to position all of the grid's child-placeholders at the IStart,BStart corner of the grid's own padding box.  (This is how we indicate the "origin" of the out-of-flow's CSS Box Alignment area, for the benefit of nsAbsoluteContainingBlock).  We also need to store the size of the grid's padding box, perhaps in the same GridItemContainingBlockRect proptable entry mentioned above -- though really we just need the size, since the position is stored as the placeholder position.)

 - Otherwise (if the grid *is* an abspos CB), we need to position all of the grid's child-placeholders at the IStart,BStart corners of their grid areas. (and we'll already need to be be finding & saving that grid area rect, per my first bullet point for 10.1 above)

 - In either case, in nsAbsoluteContainingBlock::ResolveSizeDependentOffsets (a new function being added in bug 1269046), we need to look up the alignment-rect's size (from the property table) and use it when computing the css-align-based adjustment to the abspos offsets.
(In reply to Mats Palmgren (:mats) from comment #4)
> Anyway, I'd appreciate if you take this while fixing bug 1269046,
> I suspect that the code will be very similar for flexbox/grid.

I took an initial crack at this when finishing off my patch-stack for bug 1269046, but it's not quite as straightforward as I'd thought it'd be (e.g. I thought I remembered that we already had code to resolve grid areas for abspos items -- but we don't).  So, there's a bit more grid-specific work here than I'd estimated (as broken down in comment 7).

I intend to post the remaining patches for supporting bug 1269046 tonight or tomorrow morning, and then I'm on PTO until Monday. I can circle back to this bug at that point -- but if you've got space on your plate and would be interested in taking this (the things in comment 7, on top of bug 1269046's patches), feel free!
> I thought I remembered that we already had code to resolve grid areas for abspos items -- but we don't

Is ContainingBlockForAbsPos what you're looking for?
http://searchfox.org/mozilla-central/rev/e3e8571c5378ac92663d4f583ccc4ad0a3019716/layout/generic/nsGridContainerFrame.cpp#5836-5837
Ah, probably! Thanks. I'll take a closer look tomorrow or Monday (after I've cleared my review backlog and can dive back into this).
Summary: [css-grid] abspos child of a grid doesn't respect "align-self"/"justify-self" → [css-grid] abspos child of a grid should respect "align-self"/"justify-self" when computing its static position
No longer blocks: 1268999
Duplicate of this bug: 1268999
Assignee: nobody → dholbert
Blocks: 1217086
(Note: Part 4 happens to be what fixes bug 1268999's testcase.)
(I'll include more reftests before landing, too; just wanting to get the ball rolling on review, since I'm fairly sure additional reftests won't turn up any serious issues.)
Comment on attachment 8807521 [details]
Bug 1269017 part 1 - [css-grid] Adjust OffsetToAlignedStaticPos() to use correct alignment container for abpsos children of grid containers.

https://reviewboard.mozilla.org/r/90662/#review90358

::: layout/generic/nsAbsoluteContainingBlock.cpp:370
(Diff revision 1)
> + * @param aAbsPosCBSize The abspos CB size, in terms of its own WritingMode.
>   * @param aPlaceholderContainer The parent of the child frame's corresponding

nit: ", in terms of its own WritingMode" doesn't make sense to me.
I think it's in the CB's writing-mode, so you can replace it with "(in terms of aAbsPosCBWM)" ?
Attachment #8807521 - Flags: review?(mats) → review+
Comment on attachment 8807522 [details]
Bug 1269017 part 2 - [css-grid] Give grid an implementation of CSSAlignmentForAbsPosChild().

https://reviewboard.mozilla.org/r/90664/#review90364

::: layout/generic/nsGridContainerFrame.cpp:6528
(Diff revision 1)
> +    // "the 'normal' keyword behaves as 'start' on replaced
> +    // absolutely-positioned boxes, and behaves as 'stretch' on all other
> +    // absolutely-positioned boxes."
> +    // https://drafts.csswg.org/css-align/#align-abspos
> +    // https://drafts.csswg.org/css-align/#justify-abspos
> +    alignment = aChildRI.mFrame->IsFrameOfType(nsIFrame::eReplaced) ?
> +      NS_STYLE_ALIGN_START : NS_STYLE_ALIGN_STRETCH;

Just a note: this rule makes 'normal' behave as 'start' for images, which seems a little weird to me... at least if we're using a grid area as the CB.  It means we can't have ratio-preserving stretching?
Attachment #8807522 - Flags: review?(mats) → review+
Comment on attachment 8807523 [details]
Bug 1269017 part 3 - [css-grid] Reduce scope of a mStaticPosIsCBOrigin check to *just* cover hypothetical-position calculation, and update comments.

https://reviewboard.mozilla.org/r/90666/#review90368

::: layout/generic/ReflowInput.h:731
(Diff revision 1)
> +    // for the frame in quesiton.  It's just what the caller is asking that we
> +    // use when setting up this ReflowInput.)

That we use what?
Attachment #8807523 - Flags: review?(mats) → review+
Comment on attachment 8807524 [details]
Bug 1269017 part 4 - [css-grid] When doing CSS Box Alignment, opt out of a CalculateHypotheticalPosition() clause that disregards placeholder's inline position.

https://reviewboard.mozilla.org/r/90668/#review90374

::: layout/generic/ReflowInput.cpp:1413
(Diff revision 1)
> +    // (Or if mFlags.mIOffsetsNeedCSSAlign is set, it represents the IStart
> +    // edge of the Alignment Container.)

The "IStart padding edge" I presume?
Attachment #8807524 - Flags: review?(mats) → review+
Comment on attachment 8807526 [details]
Bug 1269017 part 6 - [css-grid] Reftests for static position of abspos grid children under influence of align-self / justify-self.

https://reviewboard.mozilla.org/r/90672/#review90378

Make sure you include abs.pos. image items in the reftests.

And all possible combinations of container/item writing-modes.
My experience from writing the grid item alignment proved that
to be very useful in finding bugs.
(See layout/reftests/css-grid/grid-item-align-* for some script
that generate those combinations.  Heck, you might just want to
clone those but make the item abs.pos. instead.)
Comment on attachment 8807525 [details]
Bug 1269017 part 5 - [css-grid] Tag grid placeholder children as needing CSS Box Alignment, & consolidate their placement code.

https://reviewboard.mozilla.org/r/90670/#review90390
Attachment #8807525 - Flags: review?(mats) → review+
Comment on attachment 8807521 [details]
Bug 1269017 part 1 - [css-grid] Adjust OffsetToAlignedStaticPos() to use correct alignment container for abpsos children of grid containers.

https://reviewboard.mozilla.org/r/90662/#review90358

> nit: ", in terms of its own WritingMode" doesn't make sense to me.
> I think it's in the CB's writing-mode, so you can replace it with "(in terms of aAbsPosCBWM)" ?

Sure - fixed locally.

(While I'm at it, I made a similar tweak to the line above -- the documentation for the kid size.  I've made that explicitly reference aAbsPosCBWM, too, instead of talking about "the containing block's WritingMode". We have a dedicated line of documentation for aAbsPosCBWM further down, so no information is lost.)
Comment on attachment 8807522 [details]
Bug 1269017 part 2 - [css-grid] Give grid an implementation of CSSAlignmentForAbsPosChild().

https://reviewboard.mozilla.org/r/90664/#review90364

> Just a note: this rule makes 'normal' behave as 'start' for images, which seems a little weird to me... at least if we're using a grid area as the CB.  It means we can't have ratio-preserving stretching?

Yup -- this distinction is just to follow the letter of the spec, but it doesn't actually make a difference.  STRETCH is treated identically to START in CSSAlignUtils::AlignJustifySelf()[1] (which is appropriate since it's supposed to fall back to FLEX_START which means START since this is a grid).  So this "START or STRETCH" branch here doesn't matter in practice.
[1] https://dxr.mozilla.org/mozilla-central/rev/9c6b3b3a119bc48f5d12e3161218e98adaa12627/layout/generic/CSSAlignUtils.cpp#135-138

In other words: we're not *actually* stretching (not here at least) -- and I *think* that's correct.  css-align 6.1.3 (about static position of abspos elements) says:
> Alignment Subject:  The element’s margin box after laying out the element, treated as fixed-size for the purpose of alignment.
> 
> 'normal' Behavior:  Behaves as (the fallback for) stretch (because it is treated as fixed-size)
https://drafts.csswg.org/css-align/#align-abspos-static

Both of those lines suggest to me that no "stretch"-stretching is expected to happen.

So, I'm actually not clear under what circumstances 6.1.2's abspos-replaced-element-specific stretch-vs.-start distinction is expected to make a difference... I should probably file an issue to get clarification on that. For now, though, I think we're OK not stretching, at least in this chunk of the code.
Comment on attachment 8807523 [details]
Bug 1269017 part 3 - [css-grid] Reduce scope of a mStaticPosIsCBOrigin check to *just* cover hypothetical-position calculation, and update comments.

https://reviewboard.mozilla.org/r/90666/#review90368

> That we use what?

With that sentence, I meant "the caller is just asking us to have this behavior, despite knowing it's not actually correct", basically.

Anyway, I just removed the sentence, because it was a bit too hand-wavy anyway. So now the patch is just adding this here:
    // [...] (Note that this
    // doesn't necessarily mean that (0, 0) is the *correct* static position
    // for the frame in question.)

...and I'll let the this patch's other code-comment at the usage site in nsAbsoluteContainingBlock.cpp provide more clarity about why we're setting this flag there.
Comment on attachment 8807524 [details]
Bug 1269017 part 4 - [css-grid] When doing CSS Box Alignment, opt out of a CalculateHypotheticalPosition() clause that disregards placeholder's inline position.

https://reviewboard.mozilla.org/r/90668/#review90374

> The "IStart padding edge" I presume?

Not really -- the "padding edge of an Alignment Container" isn't a meaningful concept.

The Alignment Container is just a single rectangle, defined here:
 https://drafts.csswg.org/css-align/#alignment-container

It might technically *be* a padding box [in the case of an abspos grid child with a not-acting-as-CB grid], but it also might be a grid area [in the case of an abspos grid child with an acting-as-CB grid], or a content box [in the case of an abspos flex child].

So: "the IStart edge of the Alignment Container" is appropriately-specific here, I think.
> The Alignment Container is just a single rectangle

Oh, right, I had already forgot. :-)

(In reply to Daniel Holbert [:dholbert] from comment #27)
> Both of those lines suggest to me that no "stretch"-stretching is expected
> to happen.

Well, 6.1.2 / 6.2.2 says explicitly that we should stretch:
https://drafts.csswg.org/css-align/#justify-abspos
https://drafts.csswg.org/css-align/#align-abspos
"Note that stretch does cause replaced absolutely-positioned elements to fill
their containing block just as non-replaced ones do."

I'm reading up on these chapters now, they are quite confusing indeed... :-(
(In reply to Mats Palmgren (:mats) from comment #30)
> "Note that stretch does cause replaced absolutely-positioned elements to fill
> their containing block just as non-replaced ones do."
> 
> I'm reading up on these chapters now, they are quite confusing indeed... :-(

Yeah -- I *think* that spec-text is merely to intending to establish retroactive continuity between the traditional CSS 2.1 sizing of abspos elements with "left: 10px; right: 10px; width: auto", *in terms of modern CSS Box Alignment keywords & concepts.  It's explaining that old behavior in terms of new concepts.

Testcase (with no grid/flex/align keywords): https://jsfiddle.net/k41adkp6/
(^ updated patches to with review feedback addressed. Still need to add more reftests.)
Note that I've landed the change that deprecates 'last-baseline' now,
in case you have any tests here that needs updating.
Thanks - I've updated my tests to account for that change.
Comment on attachment 8807526 [details]
Bug 1269017 part 6 - [css-grid] Reftests for static position of abspos grid children under influence of align-self / justify-self.

https://reviewboard.mozilla.org/r/90672/#review90378

I've fashioned my reftests off of my existing abspos-flex-item reftests (rather than your scripted reftests).

I haven't covered all possible combinations, but I think I've got representative samples of all the interesting combinations. Specifically, I've included reftests that exercise the following scenarios:
 - images as grid-child
 - an RTL grid container
 - an RTL grid container with LTR child
 - a vertical-rl grid container
 - a vertical-rl grid container with horizontal-tb child
 - (and of course, grid container & child having default writing mode & direction)

For each of high-level categories, there are 4 tests:
 - a version where the grid is the abspos containing block (lower numbered tests), and a version where the viewport is the abspos containing block (higher-numbered tests).
 - a version where we're test all align-self values, and a version where we test all justify-self values. (The tested property is in the tests' filenames.)

This gives us a representative sample of the following "interesting" scenarios: grid + child parallel, grid + child antiparallel, grid + child orthogonal; as well as various configurations of abspos container + grid + child being antiparallel/orthogonal, in the scenarios where the viewport is the abspos containing block & we vary the writing-mode & direction on the grids & their children.
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5795b76480b6
part 1 - [css-grid] Adjust OffsetToAlignedStaticPos() to use correct alignment container for abpsos children of grid containers. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e6f0bf39062
part 2 - [css-grid] Give grid an implementation of CSSAlignmentForAbsPosChild(). r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab70dc086596
part 3 - [css-grid] Reduce scope of a mStaticPosIsCBOrigin check to *just* cover hypothetical-position calculation, and update comments. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2bdebbdb89
part 4 - [css-grid] When doing CSS Box Alignment, opt out of a CalculateHypotheticalPosition() clause that disregards placeholder's inline position. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/11829d96d901
part 5 - [css-grid] Tag grid placeholder children as needing CSS Box Alignment, & consolidate their placement code. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef2f15749bb9
part 6 - [css-grid] Reftests for static position of abspos grid children under influence of align-self / justify-self.
The link rel="match" in these tests all say "flex" rather than "grid", causing layout/reftest/w3c-css/submitted/check-for-references.sh to complain.  Would you mind fixing the rel="match" links?
Flags: needinfo?(dholbert)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62d5e0939949
followup: Fix typo in <link rel="match"> reference metadata, in reftests for CSS Box Alignment of abspos grid children. (test-only)
Thanks for the heads-up, and sorry about that!

I fixed the issue (see comment 52) and I ran that script locally to be sure I didn't miss any.  (I didn't know [or had forgotten] about that script -- I'll definitely use it in the future before adding new w3c-submitted tests!)
Flags: needinfo?(dholbert)
You need to log in before you can comment on or make changes to this bug.