[css-flex] Honor CSS alignment properties, when determining static position for abspos child of a flex container

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 affected, firefox52 fixed)

Details

()

Attachments

(11 attachments, 1 obsolete attachment)

58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
Assignee

Description

3 years ago
This bug is a helper for bug 874718.

This bug covers computing the static position of the flex child, while properly handling css alignment properties. Spec quote:
{
The static position of an absolutely-positioned child of a flex container is determined such that the child is positioned as if it were the sole flex item in the flex container, assuming both the child and the flex container were fixed-size boxes of their used size. For this purpose, a value of align-self: auto is treated identically to start.
}
https://drafts.csswg.org/css-flexbox-1/#abspos-items
Assignee

Updated

3 years ago
Blocks: 874718
Assignee

Comment 1

3 years ago
So currently, our abspos static-position computation code all assumes we can figure out the position *before* we've reflowed the abspos thing.  (And this has traditionally been a safe assumption.)

Specifically:
 - The parent of the abspos thing is responsible for positioning the nsPlaceholderFrame, as a zero-sized element, *well before* we've reflowed/sized the abspos element (which may not be reflowed until much later, depending on how far up the tree its abspos CB is).
 - Just before we get around to reflowing the abspos thing, we update our determination of its static position using "CalculateHypotheticalPosition()" (in nsHTMLReflowState.cpp).

However -- now this assumption is no longer valid (for abspos children of a grid/flex container at least), because the static position will now need to be influenced by the alignment properties *and the size of the abspos thing* -- e.g. "align-self:center" needs to give us a different static position, depending on how big the abspos thing is.

So, my tentative plan here is to:
 - Don't bother trying to set a useful position on nsPlaceholderFrame (not up-front at least).  Just ignore those frames during flex reflow, pretty much.
 - When we get around to reflowing the abspos thing -- in nsAbsoluteContainingBlock::ReflowAbsoluteFrame -- add an extra pass at the end, just for grid/flex items, to check the alignment properties & determine the correct static position (if we even need the static position).  And then let the result influence our already-existing "Offset the frame rect" tweak in that function.

Let me know if this sounds sane, and if there are any obvious pitfalls...
Assignee

Updated

3 years ago
Blocks: 1269017
(In reply to Daniel Holbert [:dholbert] from comment #1)
>  - Don't bother trying to set a useful position on nsPlaceholderFrame (not
> up-front at least).  Just ignore those frames during flex reflow, pretty
> much.

As long as the position is initialized (e.g., to 0).

>  - When we get around to reflowing the abspos thing -- in
> nsAbsoluteContainingBlock::ReflowAbsoluteFrame -- add an extra pass at the
> end, just for grid/flex items, to check the alignment properties & determine
> the correct static position (if we even need the static position).  And then
> let the result influence our already-existing "Offset the frame rect" tweak
> in that function.

An extra pass of what?  It doesn't seem like it should require an extra pass of reflow -- but just repositioning, right?
Assignee

Comment 3

3 years ago
Exactly right -- just some repositioning. (I meant to say "extra step", not "extra pass"; sorry for the confusing language)
> e.g. "align-self:center" needs to give us a different static position,
> depending on how big the abspos thing is.

Are you sure?  Requiring the abs.pos. frame's size when calculating
the static position when the actual abs.pos. containing block
is an ancestor of the flexbox/grid container seems very wrong to me.
Why shouldn't the static position just be "align-self:center"
applied on the placeholder?

Also, when applying align-self:center on the abs.pos. frame (in its
abs.pos. CB), why should we even use the static position at all?
Isn't the expected result that the abs.pos. frame always center itself
in its abs.pos. CB? (i.e. not the flexbox/grid parent)
Flags: needinfo?(dholbert)
OK, so now I see that 'auto' offsets turns off alignment in the CB:
https://drafts.csswg.org/css-align-3/#align-abspos
(per the last sentence under "Other Details")

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Let me know if this sounds sane, and if there are any obvious pitfalls...

So I think your plan there should work.  An alternative is to let the grid/
flexbox container align the placeholder and then ReflowAbsoluteFrame can use
that position to align the abs.pos. as needed.  Not sure which is easier to
implement.
Flags: needinfo?(dholbert)
Assignee

Comment 6

3 years ago
(In reply to Mats Palmgren (:mats) from comment #5)
> So I think your plan there should work.  An alternative is to let the grid/
> flexbox container align the placeholder and then ReflowAbsoluteFrame can use
> that position to align the abs.pos. as needed.

Hmm, good point -- I can imagine how that would work. I think you're suggesting we'd first determine the static position normally, *as if* the abspos thing were zero sized (when the parent positions its placeholder) -- and then ReflowAbsoluteFrame would offset from that position as-needed by subtracting a fraction of the frame's size after we reflow it. (0% of the size if it's left-aligned, half if it's centered, 100% if it's right-aligned)

One benefit of that alternative: it means we wouldn't have to bother looking at the parent frame's size in ReflowAbsoluteFrame.  That's not too much of a savings, though, because We would still need to inspect the parent's styling and writing mode to figure out what e.g. "align-self: flex-end" actually means (to decide how much we should offset after reflow, if at all).  This alternative plan might also mean more semi-duplicated code (depending on how hard/shoehorny it is to share the abspos-vs.-normal alignment codepaths in nsFlexContainerFrame & nsGridContainerFrame). I can imagine it being easier to do all of this in one place, in ReflowAbsoluteFrame, with perhaps a few "if grid / else if flex" conditionals.

I'll think about it some more & keep both alternatives in mind as I start working on this.
Yeah, I think we need to add a method on nsGrid/FlexContainerFrame that
takes a frame (the abs.pos.) and aligns as if it were an item, sort of.
I think my suggestion fails when it comes to overflow handling, the placeholder
would never overflow but the abs.pos. might.
https://drafts.csswg.org/css-align-3/#overflow-values
FYI, one thing I noted regarding the static-position alignment:
https://drafts.csswg.org/css-grid/#static-position
"a value of align-self: 'auto' is treated identically to 'start'."
So you need to check mAlignSelf directly for that case because
ComputedAlignSelf never returns 'auto':
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp?rev=500cb83c3626#1720
Assignee

Updated

3 years ago
Blocks: 1268999
Assignee

Comment 10

3 years ago
(Pretty much, yes.)
Comment hidden (mozreview-request)
Assignee

Updated

3 years ago
Attachment #8800413 - Attachment is obsolete: true
Attachment #8800413 - Flags: review?(mats)
Assignee

Comment 16

3 years ago
Parts 1 through 3 are pretty trivial.  Part 1 and 2 are just moving code around (and adding some new-file boilerplate & documentation, in the case of part 1). And part 3 is adjusting a grid-specific comment in the cut-and-pasted-from-grid code in CSSAlignUtils.

Functional patches coming up next.

Comment 17

3 years ago
mozreview-review
Comment on attachment 8800417 [details]
Bug 1269046 part 1: Spin out some grid alignment code into a helper method, in a new CSSAlignUtils class.

https://reviewboard.mozilla.org/r/85330/#review83912

::: layout/generic/CSSAlignUtils.h:6
(Diff revision 1)
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* Utility code for performing CSS Alignment */

nit: I understand what you mean but CSS Alignment is neither a defined term nor the spec's name.
Perhaps "Utility code for implementing the CSS Align spec" or some such would be better?
(ditto for the .cpp file if you decide to change it)

::: layout/generic/CSSAlignUtils.h:27
(Diff revision 1)
> +   *                   already converted those to an appropriate "simpler"
> +   *                   keyword.

nit: s%an appropriate "simpler" keyword%the corresponding start/end% would be clearer?
Attachment #8800417 - Flags: review?(mats) → review+

Comment 18

3 years ago
mozreview-review
Comment on attachment 8800418 [details]
Bug 1269046 part 2: Spin out a helper function to hold nsAbsoluteContainingBlock's code for resolving abspos offsets.

https://reviewboard.mozilla.org/r/85332/#review83918
Attachment #8800418 - Flags: review?(mats) → review+

Comment 19

3 years ago
mozreview-review
Comment on attachment 8800412 [details]
Bug 1269046 part 3: Make a grid-specific comment more general, in CSSAlignUtils::AlignJustifySelf.

https://reviewboard.mozilla.org/r/85326/#review83920

::: layout/generic/CSSAlignUtils.cpp:46
(Diff revision 2)
>        aAlignment = MOZ_LIKELY(aSameSide) ? NS_STYLE_ALIGN_END
>                                           : NS_STYLE_ALIGN_START;
>        break;
> -    case NS_STYLE_ALIGN_FLEX_START: // same as 'start' for Grid
> +    // flex-start/flex-end are the same as start/end, in most contexts.
> +    // (They have special behavior in flex containers, so flex containers
> +    // should resolve them to a simpler value before calling this method.)

nit: again, "simpler value" seems like an odd term, perhaps something like "so flex containers should map them to some other value before calling this method"

Comment 20

3 years ago
mozreview-review
Comment on attachment 8800412 [details]
Bug 1269046 part 3: Make a grid-specific comment more general, in CSSAlignUtils::AlignJustifySelf.

https://reviewboard.mozilla.org/r/85326/#review83922
Attachment #8800412 - Flags: review?(mats) → review+
(In reply to Mats Palmgren (:mats) from comment #17)
> > +/* Utility code for performing CSS Alignment */

Ha, now that I look that up the name is "CSS Box Alignment Module",
so "CSS Alignment" probably is more accurate than "CSS Align".
I've always called it CSS Align mostly because its tag is [css-align]
in all the comments on www-style, github issues, bugs etc.
Assignee

Comment 22

3 years ago
mozreview-review-reply
Comment on attachment 8800417 [details]
Bug 1269046 part 1: Spin out some grid alignment code into a helper method, in a new CSSAlignUtils class.

https://reviewboard.mozilla.org/r/85330/#review83912

> nit: I understand what you mean but CSS Alignment is neither a defined term nor the spec's name.
> Perhaps "Utility code for implementing the CSS Align spec" or some such would be better?
> (ditto for the .cpp file if you decide to change it)

Thanks -- I updated this comment to say "CSS Box Alignment" (in the .h and .cpp files) to make it more precise/searchable.

> nit: s%an appropriate "simpler" keyword%the corresponding start/end% would be clearer?

Updated to say:
  [...] this method expects the caller to have
  already resolved those to 'start', 'end', or 'stretch'.

(There are cases where 'auto' behaves as 'normal' which behaves as 'stretch' -- so start/end is not quite sufficient.)
Assignee

Comment 23

3 years ago
mozreview-review-reply
Comment on attachment 8800412 [details]
Bug 1269046 part 3: Make a grid-specific comment more general, in CSSAlignUtils::AlignJustifySelf.

https://reviewboard.mozilla.org/r/85326/#review83920

> nit: again, "simpler value" seems like an odd term, perhaps something like "so flex containers should map them to some other value before calling this method"

Thanks - I fixed that locally (using your suggested language).
Assignee

Comment 24

3 years ago
Here's the high-level plan for the remaining patches here:

 (A) During nsFlexContainerFrame and nsGridContainerFrame reflow, we'll toggle a placeholder-specific state bit (tentatively called PLACEHOLDER_STATICPOS_NEEDS_CSSALIGN) on all of their placeholder children.  (This part is just an optimization -- it lets us skip a GetType() virtual-function-call later on, and hence lets us avoid slowing down the non-grid/flex codepath with that virtual call.)

 (B) In ReflowInput::InitAbsoluteConstraints, we'll check for that ^ bit on the placeholder frame.  If it's set [and we're actually *using* the static position for layout, instead of e.g. "top:0;left:0"], then we set a per-axis bit in ReflowInput::mFlags, to indicate that the static position needs to be determined (with css box alignment) in that axis.

 (C) In nsAbsoluteContainingBlock::ResolveSizeDependentOffsets (the function that I spun out in Part 2 here), we'll check those new ReflowInput::mFlags bits that I discussed in (B). If the bit is set for a given axis, we'll ask the placeholder's parent frame (the grid/flex container) for the CSS Box Alignment constant that we should use for aligning the child in the given axis. (For grid, this will be the child's {align,justify}-self value.  For flexbox, it might be from align-self, or it might be from the container's {align,justify}-content -- but we can use the *-content fallback values to "simplify" any fancy value into a different value that would be valid for align/justify-self.)

 (D) Then, nsAbsoluteContainingBlock::ResolveSizeDependentOffsets will invoke CSSAlignUtils::AlignJustifySelf() (added in part 1) using that alignment constant, along with the child's size & the container's size.  This will give it the aligned position within the container, and it'll update aOffsets accordingly.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 28

3 years ago
(^ those mozreview pushes are just rebased versions of the parts that are already posted here)
Assignee

Updated

3 years ago
Depends on: 1311865
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 32

3 years ago
(^ sorry for bugspam -- that's just one more rebase, to layer these on top of bug 1311865. (Previously I had them in the opposite order, before I decided to spin bug 1311865 off to its own bug.))
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 41

3 years ago
(In reply to Daniel Holbert [:dholbert] (PTO Oct 21-25) from comment #24)
> Here's the high-level plan for the remaining patches here:
> 
>  (A) During nsFlexContainerFrame and nsGridContainerFrame reflow, we'll
> toggle a placeholder-specific state bit (tentatively called
> PLACEHOLDER_STATICPOS_NEEDS_CSSALIGN) on all of their placeholder children. 
> (This part is just an optimization -- it lets us skip a GetType()
> virtual-function-call later on, and hence lets us avoid slowing down the
> non-grid/flex codepath with that virtual call.)
> 
>  (B) In ReflowInput::InitAbsoluteConstraints, we'll check for that ^ bit on
> the placeholder frame.  If it's set [and we're actually *using* the static
> position for layout, instead of e.g. "top:0;left:0"], then we set a per-axis
> bit in ReflowInput::mFlags, to indicate that the static position needs to be
> determined (with css box alignment) in that axis.

Part 4 does both (A) and (B).

>  (C) In nsAbsoluteContainingBlock::ResolveSizeDependentOffsets (the function
> that I spun out in Part 2 here), we'll check those new ReflowInput::mFlags
> bits that I discussed in (B). If the bit is set for a given axis, we'll ask
> the placeholder's parent frame (the grid/flex container) for the CSS Box
> Alignment constant that we should use for aligning the child in the given
> axis. (For grid, this will be the child's {align,justify}-self value.  For
> flexbox, it might be from align-self, or it might be from the container's
> {align,justify}-content -- but we can use the *-content fallback values to
> "simplify" any fancy value into a different value that would be valid for
> align/justify-self.)
> 
>  (D) Then, nsAbsoluteContainingBlock::ResolveSizeDependentOffsets will
> invoke CSSAlignUtils::AlignJustifySelf() (added in part 1) using that
> alignment constant, along with the child's size & the container's size. 
> This will give it the aligned position within the container, and it'll
> update aOffsets accordingly.

Part 5 does (C) and (D) -- except that the nsFlexContainerFrame impl of "ask for the CSS Box Alignment constant" comes in part 7 (with a supporting patch in part 6, which just lets us optionally-disable an FlexboxAxisTracker hack that I added in bug 983427 which we don't need here & hence just needlessly complicates things.)

Comment 42

3 years ago
mozreview-review
Comment on attachment 8803262 [details]
Bug 1269046 part 4: Set flags on nsPlaceholderFrame & ReflowInput to track abspos frames that need CSS Box Alignment to resolve static position.

https://reviewboard.mozilla.org/r/87426/#review86622

r=mats with the issues resolved

::: layout/generic/ReflowInput.h:222
(Diff revision 1)
>                                          // reflow its placeholder children.
>      uint16_t mShrinkWrap:1; // stores the COMPUTE_SIZE_SHRINK_WRAP ctor flag
>      uint16_t mUseAutoBSize:1; // stores the COMPUTE_SIZE_USE_AUTO_BSIZE ctor flag
>      uint16_t mStaticPosIsCBOrigin:1; // the STATIC_POS_IS_CB_ORIGIN ctor flag
> +
> +    // If set, the following 2 flags indicate that:

nit: s/2/two/

::: layout/generic/ReflowInput.h:237
(Diff revision 1)
> +    uint16_t mIOffsetsNeedCSSAlign:1;
> +    uint16_t mBOffsetsNeedCSSAlign:1;

If I counted correctly, all 16 bits are already in use before this patch.  So I think we should change the type to uint32_t for all the bits now.

::: layout/generic/ReflowInput.cpp:1571
(Diff revision 1)
>          aPresContext->PresShell()->GetPlaceholderFrameFor(mFrame);
>        NS_ASSERTION(placeholderFrame, "no placeholder frame");
>        CalculateHypotheticalPosition(aPresContext, placeholderFrame, cbrs,
>                                      hypotheticalPos, aFrameType);
> +
> +      if (placeholderFrame->HasAnyStateBits(PLACEHOLDER_STATICPOS_NEEDS_CSSALIGN)) {

(Speculation: I suspect that it might be better to move this block one level out so that it also runs for Grid.  I don't think we'll ever want to call CalculateHypotheticalPosition for Grid since the CB is the grid area.  Anyway, we can move it later as needed.)

::: layout/generic/ReflowInput.cpp:1572
(Diff revision 1)
>        NS_ASSERTION(placeholderFrame, "no placeholder frame");
>        CalculateHypotheticalPosition(aPresContext, placeholderFrame, cbrs,
>                                      hypotheticalPos, aFrameType);
> +
> +      if (placeholderFrame->HasAnyStateBits(PLACEHOLDER_STATICPOS_NEEDS_CSSALIGN)) {
> +        nsIFrame* placeholderParent = placeholderFrame->GetParent();

Needs DebugOnly<> to avoid warning?

::: layout/generic/nsFrameStateBits.h:595
(Diff revision 1)
>  FRAME_STATE_BIT(Placeholder, 22, PLACEHOLDER_FOR_FIXEDPOS)
>  FRAME_STATE_BIT(Placeholder, 23, PLACEHOLDER_FOR_POPUP)
>  FRAME_STATE_BIT(Placeholder, 24, PLACEHOLDER_FOR_TOPLAYER)
>  
> +// This bit indicates that the out-of-flow frame's static position needs to be
> +// determined using the CSS Alignment properties (align/justify-self, etc).

I think we should drop the ", etc" since it just makes me wonder what other properties are involved here, but there are none afaik.  I'm guessing you mean there are -webkit- aliases that maps to these but that's not a concern here I think.

::: layout/generic/nsFrameStateBits.h:598
(Diff revision 1)
>  
> +// This bit indicates that the out-of-flow frame's static position needs to be
> +// determined using the CSS Alignment properties (align/justify-self, etc).
> +// When this is set, the placeholder frame's position doesn't represent the
> +// static position, as it usually would -- rather, it represents the logical
> +// "start" corner of the alignment containing block.  Then, after we've

nit: s/"start"/start  (preceding it with "logical" makes it clear without quotes)
Attachment #8803262 - Flags: review?(mats) → review+
Assignee

Comment 43

3 years ago
Thanks! I'm pushing a quick update to fix the DebugOnly thing you noted, and to remove some set-but-unused variables that Try (with g++ rather than clang) caught in Part 5. (You may have noticed them in review, too -- sorry if they caused confusion / unnecessary-review-processing).

I'll address review comments soon; about to take off on a flight right now, though. :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 49

3 years ago
mozreview-review
Comment on attachment 8803263 [details]
Bug 1269046 part 5: If an abspos child's offset depends on CSS Box Alignment, ask nsContainerFrame for the alignment enum to use, and align with CSSAlignUtils.

https://reviewboard.mozilla.org/r/87574/#review86664

r=mats with the issues addressed

::: layout/generic/nsAbsoluteContainingBlock.cpp:358
(Diff revision 1)
> + * This function returns the length that we need to shift a given abs/fixed-pos
> + * child away from the "start" corner of its CSS Box Alignment area, in order

nit: rather than 'length that we need to shift...away from the "start" corner', can we just say it returns the offset from the start corner?

::: layout/generic/nsAbsoluteContainingBlock.cpp:383
(Diff revision 1)
> +                         const LogicalSize& aKidSizeInAbsPosCBWM,
> +                         nsContainerFrame* aPlaceholderContainer,
> +                         WritingMode aAbsPosCBWM,
> +                         LogicalAxis aAbsPosCBAxis)
> +{
> +  // (Most of this function is simply preparing args that we'll pass to

I think we should add this, just in case:
if (!aPlaceholderContainer) {
  return 0;
}

(the callers later in this patch pass the result from GetPlaceholderContainer unchecked)

::: layout/generic/nsAbsoluteContainingBlock.cpp:387
(Diff revision 1)
> +  // case of grid, it's an area that shares a writing-mode with
> +  // aPlaceholderContainer). So, we'll most of our arithmetic/alignment in the

(I don't think the grid area has any relationship with the placeholder container's WM currently (it's in the grid container's WM IIRC).  But since this code doesn't handle grid yet, I guess we can fix this later.)

::: layout/generic/nsAbsoluteContainingBlock.cpp:412
(Diff revision 1)
> +  const LogicalSize kidSizePCWM = aKidSizeInAbsPosCBWM.ConvertTo(pcWM,
> +                                                                 aAbsPosCBWM);

kidSizePCWM is only used to calculate kidSizeInAxis, but kidSizeInAxis is never actually used after its been assigned.  Just remove these two variables?

::: layout/generic/nsAbsoluteContainingBlock.cpp:428
(Diff revision 1)
> +  // as if "unsafe" was the specified value (which is basically equivalent to
> +  // the default behavior, when no value is specified -- though the default
> +  // behavior also has some [at-risk] extra nuance about scroll containers...)
> +  const bool overflowSafe = false;
> +
> +  uint8_t alignConst =

Please make this uint16_t and then strip off the NS_STYLE_ALIGN_FLAG_BITS here.  I think we should add support for <overflow-position> here soon-ish and it seems better to have the signatures/types correct from the start.

::: layout/generic/nsAbsoluteContainingBlock.cpp:430
(Diff revision 1)
> +  // behavior also has some [at-risk] extra nuance about scroll containers...)
> +  const bool overflowSafe = false;
> +
> +  uint8_t alignConst =
> +    aPlaceholderContainer->CSSAlignmentForAbsPosChild(aKidReflowInput, pcAxis);
> +  // Find out if placeholder-container & the OOF child have the same polarity

nit: "same polarity" is a bit obscure - perhaps "start sides coincide" ?

::: layout/generic/nsAbsoluteContainingBlock.cpp:441
(Diff revision 1)
> +  // converted 'baseline'/'last-baseline' enums to their fallback values.)
> +  const nscoord baselineAdjust = nscoord(0);
> +
> +  // AlignJustifySelf operates in the kid's writing mode, so we need to
> +  // represent the child's size and the desired axis in that writing mode:
> +  LogicalSize kidSizeInOwnWM = aKidSizeInAbsPosCBWM.ConvertTo(kidWM,

nit: after removing the variables above, I think this could become just "kidSize".

::: layout/generic/nsAbsoluteContainingBlock.cpp:446
(Diff revision 1)
> +  LogicalSize kidSizeInOwnWM = aKidSizeInAbsPosCBWM.ConvertTo(kidWM,
> +                                                              aAbsPosCBWM);
> +  LogicalAxis kidAxis = (kidWM.IsOrthogonalTo(aAbsPosCBWM)
> +                         ? GetOrthogonalAxis(aAbsPosCBAxis)
> +                         : aAbsPosCBAxis);
> +

nit: remove this empty line

::: layout/generic/nsAbsoluteContainingBlock.cpp:503
(Diff revision 1)
>  
> +    // These variables are used in each of the m{I,B}OffsetsNeedCSSAlign
> +    // clauses. We declare them at this scope so we can avoid having to look
> +    // them up twice (and so we can only look them up if we need them).
> +    nsContainerFrame* placeholderContainer = nullptr;
> +    Maybe<LogicalSize> logicalCBSizeOuterWM;

AFAICT, all the branches needs this so you can just drop the Maybe<> here and calculate it upfront, rather than having "aLogicalCBSize->ConvertTo(outerWM, wm)" in four places.

::: layout/generic/nsContainerFrame.h:518
(Diff revision 1)
> +  /**
> +   * Returns a CSS Box Alignment constant which the caller can use to align
> +   * the absolutely-positioned child (whose ReflowInput is aChildRI) within
> +   * a CSS Box Alignment area associated with this container.
> +   *
> +   * The returned enum value is guaranteed to be valid as an argument to

As I said above, I'd prefer if this method returns a uint16_t (including a possible <overflow-position>), so these two lines should be removed.
Modify the XXX comment later instead to say that <overflow-position> is ignored for now since it's not implemented in CSSAlignUtils::AlignJustifySelf() yet.

::: layout/generic/nsContainerFrame.h:534
(Diff revision 1)
> +   * @param aLogicalAxis The axis (of this container frame) in which the caller
> +   *                     would like to align the child frame.
> +   *
> +   * XXXdholbert Might need to also return a <overflow-position> (bug 1311892).
> +   */
> +  virtual uint8_t CSSAlignmentForAbsPosChild(const ReflowInput& aChildRI,

Could this method be "const" ?
Attachment #8803263 - Flags: review?(mats) → review+

Comment 50

3 years ago
mozreview-review
Comment on attachment 8803264 [details]
Bug 1269046 part 6: Add a flags enum-class to customize FlexboxAxisTracker behavior.

https://reviewboard.mozilla.org/r/87576/#review86682

::: layout/generic/nsFlexContainerFrame.cpp:240
(Diff revision 1)
>  #define GET_CROSS_COMPONENT_LOGICAL(axisTracker_, wm_, isize_, bsize_)  \
>    wm_.IsOrthogonalTo(axisTracker_.GetWritingMode()) != \
>      (axisTracker_).IsRowOriented() ? (bsize_) : (isize_)
>  
> +// Flags to customize behavior of the FlexboxAxisTracker constructor:
> +enum FATFlags {

nit: Is FAT an acronym that's already in use in this file?  If not, perhaps TrackerFlags or AxisTrackerFlags is a better name?
Attachment #8803264 - Flags: review?(mats) → review+

Comment 51

3 years ago
mozreview-review
Comment on attachment 8803265 [details]
Bug 1269046 part 7: Give nsFlexContainerFrame a CSSAlignmentForAbsPosChild() implementation (to determine appropriate align enum for abspos children).

https://reviewboard.mozilla.org/r/87578/#review86686

::: layout/generic/nsFlexContainerFrame.cpp:1198
(Diff revision 1)
> +  specified &= ~NS_STYLE_ALIGN_FLAG_BITS;
> +
> +  // FIRST: handle a special-case for "justify-content:stretch" (or equivalent),
> +  // which requires that we ignore any author-provided explicit fallback value.
> +  if (specified == NS_STYLE_ALIGN_NORMAL) {
> +    // In a flex container, for *-content: "'normal' behaves as 'stretch'".

nit: drop the word "for" ?

::: layout/generic/nsFlexContainerFrame.cpp:1199
(Diff revision 1)
> +
> +  // FIRST: handle a special-case for "justify-content:stretch" (or equivalent),
> +  // which requires that we ignore any author-provided explicit fallback value.
> +  if (specified == NS_STYLE_ALIGN_NORMAL) {
> +    // In a flex container, for *-content: "'normal' behaves as 'stretch'".
> +    // Do that conversion early, so it benefits from our "stretch" special-case.

nit: s/"stretch"/'stretch'/ for consistency with surrounding text

::: layout/generic/nsFlexContainerFrame.cpp:1211
(Diff revision 1)
> +    // https://drafts.csswg.org/css-align-3/#distribution-flex
> +    // So, we just directly return 'flex-start', & ignore explicit fallback..
> +    return NS_STYLE_ALIGN_FLEX_START;
> +  }
> +
> +  // OK -- now, we have to check for an explicit fallback value (and if it's

nit: s/OK -- now, we have to check/Now check/ might read better?

::: layout/generic/nsFlexContainerFrame.cpp:1251
(Diff revision 1)
> +  bool isAxisReversed;
> +
> +  uint8_t alignment;
> +  if (isMainAxis) {
> +    isAxisReversed = axisTracker.IsMainAxisReversed();

Might be clearer as:
const bool isAxisReversed = isMainAxis ? axisTracker.IsMainAxisReversed()
                                       : axisTracker.IsCrossAxisReversed();

::: layout/generic/nsFlexContainerFrame.cpp:1288
(Diff revision 1)
> +          NS_STYLE_ALIGN_START : NS_STYLE_ALIGN_STRETCH;
> +      }
> +    }
> +  }
> +
> +  // Resolve flex-start, flex-end, "auto", left, right, baseline, last-baseline;

nit: remove the quotes from "auto"
Attachment #8803265 - Flags: review?(mats) → review+
Depends on: 1312295
Assignee

Comment 52

3 years ago
mozreview-review-reply
Comment on attachment 8803262 [details]
Bug 1269046 part 4: Set flags on nsPlaceholderFrame & ReflowInput to track abspos frames that need CSS Box Alignment to resolve static position.

https://reviewboard.mozilla.org/r/87426/#review86622

> nit: s/2/two/

Fixed.

> If I counted correctly, all 16 bits are already in use before this patch.  So I think we should change the type to uint32_t for all the bits now.

You've now fixed this for existing bits in bug 1312295 (thanks!), and I've rebased locally & am using 32 instead of 16 for these new variables now.

> Needs DebugOnly<> to avoid warning?

Fixed, per comment 43.

> I think we should drop the ", etc" since it just makes me wonder what other properties are involved here, but there are none afaik.  I'm guessing you mean there are -webkit- aliases that maps to these but that's not a concern here I think.

Gotcha -- sorry, that "etc" was meant to hint at "align/justify-content".

I'll clarify this to say "CSS *Box* Alignment properties" for consistency with other comment updates, and I'll also update the etc-parenthetical to say "[align,justify]-[self,content]". (A bit messy, but it's a parenthetical anyway, whose main goal is to clarify that this we're *not* talking about other align-ish properties like e.g. "text-align".)

> nit: s/"start"/start  (preceding it with "logical" makes it clear without quotes)

Fixed.
Assignee

Comment 53

3 years ago
mozreview-review-reply
Comment on attachment 8803263 [details]
Bug 1269046 part 5: If an abspos child's offset depends on CSS Box Alignment, ask nsContainerFrame for the alignment enum to use, and align with CSSAlignUtils.

https://reviewboard.mozilla.org/r/87574/#review86664

> nit: rather than 'length that we need to shift...away from the "start" corner', can we just say it returns the offset from the start corner?

Sure -- fixed locally to say:

 * This function returns the offset of an abs/fixed-pos child's static
 * position, with respect to the "start" corner of its CSS Box Alignment area,
 * according to CSS Box Alignment.

> I think we should add this, just in case:
> if (!aPlaceholderContainer) {
>   return 0;
> }
> 
> (the callers later in this patch pass the result from GetPlaceholderContainer unchecked)

Good point! I added a NS_ERROR and an explanatory comment, too, since if we hit this new early-return, something is very wrong. I've added the following:

  if (!aPlaceholderContainer) {
    // (The placeholder container should be the thing that kicks this whole
    // process off, by setting PLACEHOLDER_STATICPOS_NEEDS_CSSALIGN.  So it
    // should exist... but bail gracefully if it doesn't.)
    NS_ERROR("Missing placeholder-container when computing a "
             "CSS Box Alignment static position");
    return 0;
  }

> (I don't think the grid area has any relationship with the placeholder container's WM currently (it's in the grid container's WM IIRC).  But since this code doesn't handle grid yet, I guess we can fix this later.)

I clarified this language a bit:
  // NOTE: Our CSS Box Alignment area is aPlaceholderContainer's content-box
  // (or an area within it, if aPlaceholderContainer is a grid). So, we'll
  // [...]

We can clarify further in bug 1269017 if needed.

> kidSizePCWM is only used to calculate kidSizeInAxis, but kidSizeInAxis is never actually used after its been assigned.  Just remove these two variables?

You're right! Sorry about that. GCC actually pointed this out, too, when I gave this a multi-platform Try run (via its set-but-unused warnings).  (I use clang locally, rather than gcc.)

These were vestigial, from an older version of the patch.  I've removed them now.

> Please make this uint16_t and then strip off the NS_STYLE_ALIGN_FLAG_BITS here.  I think we should add support for <overflow-position> here soon-ish and it seems better to have the signatures/types correct from the start.

Done. (I added an XXX comment pointing to bug 1311892, too.)

> nit: "same polarity" is a bit obscure - perhaps "start sides coincide" ?

That suggested phrase ("start sides coincide") would require a bit more wordsmithing of the rest of this comment -- I just made a more targeted fix, replacing "polarity" with "start-sides".  It ends up:
  ...have the same start-sides in the placeholder-container's pcAxis.

> nit: after removing the variables above, I think this could become just "kidSize".

I'm going to leave it with the more wordy name, to clarify the difference ( / relationship) between this variable (kidSizeInOwnWM) and the related function-parameter (aKidSizeInAbsPosCBWM).

> nit: remove this empty line

Done.

> AFAICT, all the branches needs this so you can just drop the Maybe<> here and calculate it upfront, rather than having "aLogicalCBSize->ConvertTo(outerWM, wm)" in four places.

Good point! Fixed.

> As I said above, I'd prefer if this method returns a uint16_t (including a possible <overflow-position>), so these two lines should be removed.
> Modify the XXX comment later instead to say that <overflow-position> is ignored for now since it's not implemented in CSSAlignUtils::AlignJustifySelf() yet.

OK -- I updated the function to return uint16_t, and I replaced these two lines of documentation with:
   * The lower 8 bits of the returned value are guaranteed to form a valid
   * argument for CSSAlignUtils::AlignJustifySelf(). (The upper 8 bits may
   * encode an <overflow-position>.)

I added an XXX comment at the callsite (about honoring any returned <overflow-position>), and I dropped the later XXX comment from this documentation, since this function isn't where we do the ignoring -- it's at the callsite [and potentially in subclasses' overrides of this function]

> Could this method be "const" ?

Yup! Done.
Assignee

Comment 54

3 years ago
mozreview-review-reply
Comment on attachment 8803264 [details]
Bug 1269046 part 6: Add a flags enum-class to customize FlexboxAxisTracker behavior.

https://reviewboard.mozilla.org/r/87576/#review86682

> nit: Is FAT an acronym that's already in use in this file?  If not, perhaps TrackerFlags or AxisTrackerFlags is a better name?

AxisTrackerFlags is indeed a better name -- I'll use that, thanks!

(FAT is not an acronym that's previously used in this file; I was just avoiding the lengthy strawman-naming of e.g. "FlexboxAxisTrackerFlags::eAllowBottomToTopChildOrdering")
Assignee

Comment 55

3 years ago
mozreview-review-reply
Comment on attachment 8803265 [details]
Bug 1269046 part 7: Give nsFlexContainerFrame a CSSAlignmentForAbsPosChild() implementation (to determine appropriate align enum for abspos children).

https://reviewboard.mozilla.org/r/87578/#review86686

> nit: drop the word "for" ?

Done.

> nit: s/"stretch"/'stretch'/ for consistency with surrounding text

Done.

> nit: s/OK -- now, we have to check/Now check/ might read better?

Done.

> Might be clearer as:
> const bool isAxisReversed = isMainAxis ? axisTracker.IsMainAxisReversed()
>                                        : axisTracker.IsCrossAxisReversed();

Done. (I was trying share the existing "isMainAxis" check, instead of checking that bool twice.  But yeah, it's probably more readable this way, and the compiler can probably collapse the checks together if it likes.)

> nit: remove the quotes from "auto"

Done.
(In reply to Daniel Holbert [:dholbert] from comment #53)
>  * position, with respect to the "start" corner of its CSS Box Alignment area,
>  * according to CSS Box Alignment.

"CSS Box Alignment area" sounds a bit odd.  Isn't this the same as the Containing Block
in all cases?  (If so, Id prefer that term instead since it's a defined CSS term that
we use frequently in other code comments).  (I can't find the words "alignment area"
anywhere in CSS Align, fwiw.)
Assignee

Comment 57

3 years ago
Looks like "Alignment Container" is the term I meant to use here:
  https://drafts.csswg.org/css-align/#alignment-container

(It's not quite the same thing as the containing block -- e.g. for an abspos child of a "position:static" flex container, the Containing Block is the viewport (or the nearest positioned ancestor), but the Alignment Container is the flex container's content-box.)
Assignee

Comment 58

3 years ago
I've now fixed a bunch of "box alignment area" / "css box alignment area" comments in Part 5 to say "alignment container" instead, since that's a well-defined term.  (Including the code-comment quoted in Comment 56.)

I'm pretty sure that caught all such undefined "align area" usages in my patch-stack here.
Good, that's better since it's a defined term in CSS Align itself.
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)
So, this is *only* for aligning the static-position, right?
I mean, CSS Align is quite clear that the Alignment Container is the CB:
https://drafts.csswg.org/css-align/#justify-abspos
But, Flexbox (and Grid I assume), modifies this for the static-position case:
https://drafts.csswg.org/css-flexbox/#abspos-items
(but only in that case, IIUC)

Hmm, so for left|right:auto;top:0; and align|justify-self:center we will center
the inline axis in the Grid container, but center in the Viewport in the block-
axis?
Assignee

Comment 69

3 years ago
(In reply to Mats Palmgren (:mats) from comment #68)
> So, this is *only* for aligning the static-position, right?

Yes.

> I mean, CSS Align is quite clear that the Alignment Container is the CB:
> https://drafts.csswg.org/css-align/#justify-abspos

Huh. I'm not sure what that chunk of the spec is supposed to be about -- I think it might be about scenarios with abspos *grandchildren* (or further) of a grid container... This case:
  https://drafts.csswg.org/css-grid-1/#abspos-items

The chunk right below it (6.1.3, on "Static Position of Absolutely-Positioned Boxes") says:
  #  Alignment Container  The element’s static-position rectangle,
  #                       as defined by its parent’s layout mode.
https://drafts.csswg.org/css-align/#justify-abspos-static

That part (6.1.3) is what this bug is about.

> But, Flexbox (and Grid I assume), modifies this for the static-position case:
> https://drafts.csswg.org/css-flexbox/#abspos-items
> (but only in that case, IIUC)

Right. They're filling in the "as defined by its parent’s layout mode" definition, which css-align spec 6.1.3 (quoted above) is referring to.

> Hmm, so for left|right:auto;top:0; and align|justify-self:center we will
> center the inline axis in the Grid container, but center in the Viewport
> in the block-axis?

Not quite. Here's a jsfiddle with that scenario, in a flex container:
 https://jsfiddle.net/hcyopkte/

* "left|right:auto" make us use the static position in the horizontal axis. (centered horizontally within the flex container)
* "top:0" make us snap to the top of the containing block.

In a grid, I expect it'd be similar.
Assignee

Updated

3 years ago
No longer blocks: 1268999

Comment 71

3 years ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/539db74e4a88
part 1: Spin out some grid alignment code into a helper method, in a new CSSAlignUtils class. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb4b52bf06ec
part 2: Spin out a helper function to hold nsAbsoluteContainingBlock's code for resolving abspos offsets. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/eea6479452f0
part 3: Make a grid-specific comment more general, in CSSAlignUtils::AlignJustifySelf. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/8088e5a9e6e3
part 4: Set flags on nsPlaceholderFrame & ReflowInput to track abspos frames that need CSS Box Alignment to resolve static position. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb7655f3e1cf
part 5: If an abspos child's offset depends on CSS Box Alignment, ask nsContainerFrame for the alignment enum to use, and align with CSSAlignUtils. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/75b58c4e61e6
part 6: Add a flags enum-class to customize FlexboxAxisTracker behavior. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0008dd33cf4
part 7: Give nsFlexContainerFrame a CSSAlignmentForAbsPosChild() implementation (to determine appropriate align enum for abspos children). r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/16db55b642a9
part 8: Add reftests for CSS Alignment of absolutely positioned flex children.
Assignee

Comment 73

3 years ago
The just-landed "v2" patch on bug 1313560 should avoid (unified-build + windows.h-macro-caused) bustage on Windows.

Try run to validate that:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=62702753b0f7bb5332946a33e72160f0382d1cc6
Assignee

Comment 74

3 years ago
That Try run had some red dt mochitest runs (mainly on Windows) from overflowing the log, due to this NS_ERROR being super-spammy:
> ###!!! ASSERTION: Unsupported container for abpsos CSS Box Alignment: 'Error', file layout/generic/nsAbsoluteContainingBlock.cpp, line 416

This is an assertion that we're not expecting to ever fire. It turns out it's happening because we initialize ReflowInput::mFlags by cloning the parent's ReflowInput::mFlags.  So in cases where we have <flex><abspos block><abspos block>, the innermost abspos block incorrectly gets these new ReflowInput flags.

I've now added a crashtest that exercises this scenario (and fails due to this assertion, with my previous patches), and I've updated the "add flags" patch (part 4) to unconditionally clear these new mFlags, just before we conditionally toggle them to true.  (This fixes the assertions.)

New Try run with that fix:
https://treeherder.mozilla.org/logviewer.html#?job_id=30065792&repo=try#L229-L677539
Good catch!  I think that probably applies to the clamp bits I'm adding too.
Hmm, we should probably audit all the bits and see if others were supposed
to be "non-inherited" too...
+  // Clear any m{I,B}OffsetsNeedCSSAlign flags that we copied from parent.
+  mFlags.mIOffsetsNeedCSSAlign = mFlags.mBOffsetsNeedCSSAlign = false;

Maybe move that to the ctor where we copy mFlags from the parent instead?
I know the bits are only used for abs.pos. but it seems more correct
to reset these bits immediately in the ctor.
(in the list under "Note: mFlags was initialized as a copy..." in that ctor)

(BTW, I think we're good with the clamp bits I'm adding because those are
unconditionally set from the aFlags arg passed to the ctor, overwriting
those bits from the parent state.)
Assignee

Comment 78

3 years ago
(In reply to Mats Palmgren (:mats) from comment #77)
> Maybe move that to the ctor where we copy mFlags from the parent instead?
> (in the list under "Note: mFlags was initialized as a copy..." in that ctor)

Perfect, thanks - that's indeed the right place to clear them. Fixed locally.
Assignee

Comment 79

3 years ago
I noticed two more margin-related issues that I want to fix before re-landing. So -- I'm posting a few more small patches for review.

Specifically, the issues addressed in the new patches are:
 A) "auto" margins -- the spec is a bit vague on whether to expand vs. ignore these, when determining the static position of abspos elements.  Right now, my patch stack expands them (via the CSSAlignJustifyUtils::AlignJustifySelf impl, taken from grid).  But, other UAs (Chrome & Edge) don't expand them on abspos flex children, and (similarly) neither we nor they expand horizontal auto margins on abspos block-level children of a block.  So, for consistency with the block behavior & for interop with other UAs, we should avoid expanding auto margins when determining static position of these children.  SO: the new "part 9" makes auto-margin-expansion an opt-in behavior in CSSAlignUtils::AlignJustifySelf().  (And "part 8" is a helper idempotent refactoring, to change the existing AlignJustifySelf() boolean args into a bitfield.)  I also filed https://github.com/w3c/csswg-drafts/issues/665 on clarifying the spec on this -- and we can easily change behavior if the spec editors say that we should be expanding these margins.

 B) non-auto margins: In the previous patch-stack, we were accidentally applying margins twice!
  - Once in the new codepath, inside of CSSAlignUtils::AlignJustifySelf() (since that really gives us the offset to align the kid's *margin box*, as it needs to do)
  - ...and then again, separately in the normal/existing codepath, in nsAbsoluteContainingBlock.cpp.
SO: The new "part 10" patch makes use clear out the margins that are used in the normal codepath, if we're doing CSS Box Alignment (in which case it will have applied the margins for us).

I've included new tests in the updated reftests patch, to test both of these behaviors.
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 91

3 years ago
mozreview-review
Comment on attachment 8803263 [details]
Bug 1269046 part 5: If an abspos child's offset depends on CSS Box Alignment, ask nsContainerFrame for the alignment enum to use, and align with CSSAlignUtils.

https://reviewboard.mozilla.org/r/87574/#review88740

::: layout/generic/nsAbsoluteContainingBlock.cpp:554
(Diff revision 4)
>      didResolveOffsets = true;
>    }
>  
>    if (didResolveOffsets) {
>      aKidReflowInput.SetComputedLogicalOffsets(aOffsets->ConvertTo(wm, outerWM));
>    }

I don't see the point of |didResolveOffsets|, can't we just move the SetComputedLogicalOffsets call to where the assignment is instead?

Comment 92

3 years ago
mozreview-review
Comment on attachment 8805858 [details]
Bug 1269046 part 8: Rewrite CSSAlignUtils::AlignJustifySelf to use a single "flags" arg instead of boolean args.

https://reviewboard.mozilla.org/r/89480/#review88744

::: layout/generic/CSSAlignUtils.h:22
(Diff revision 1)
>    /**
> +   * Flags to customize the behavior of AlignJustifySelf:
> +   */
> +  enum class AlignJustifyFlags {
> +    eNoFlags           = 0,
> +    // Indicates that we have <overflow-position> = safe:

minor nit: s/safe:/safe./ for consistency with the comments for the other two bits
Attachment #8805858 - Flags: review?(mats) → review+

Comment 93

3 years ago
mozreview-review
Comment on attachment 8805859 [details]
Bug 1269046 part 9: Add a flag to control whether CSSAlignUtils::AlignJustifySelf() expands "auto" margins.

https://reviewboard.mozilla.org/r/89482/#review88746

::: layout/generic/CSSAlignUtils.h:29
(Diff revision 1)
> +    eExpandAutoMargins = 1 << 2,
>    };

I'd prefer to reverse the logic of this bit if possible.  It seems like expanding auto-margins should be the default behavior.  Suppressing auto-margins here seems like a tweak for nsAbsoluteContainingBlock that other consumers shouldn't be penalized for.
Attachment #8805859 - Flags: review?(mats) → review-

Comment 94

3 years ago
mozreview-review
Comment on attachment 8805860 [details]
Bug 1269046 part 10: Skip the normal margin-adding codepath for abspos frames that undergo CSS Box Alignment (which takes care of margins).

https://reviewboard.mozilla.org/r/89484/#review88748
Attachment #8805860 - Flags: review?(mats) → review+
Assignee

Comment 95

3 years ago
mozreview-review-reply
Comment on attachment 8803263 [details]
Bug 1269046 part 5: If an abspos child's offset depends on CSS Box Alignment, ask nsContainerFrame for the alignment enum to use, and align with CSSAlignUtils.

https://reviewboard.mozilla.org/r/87574/#review88740

> I don't see the point of |didResolveOffsets|, can't we just move the SetComputedLogicalOffsets call to where the assignment is instead?

Yup -- thanks, I'll fix that. (I think there were previously several different cases where we set it to true - but now it's only set to true in one place, as you noticed.)
Assignee

Comment 96

3 years ago
mozreview-review-reply
Comment on attachment 8805859 [details]
Bug 1269046 part 9: Add a flag to control whether CSSAlignUtils::AlignJustifySelf() expands "auto" margins.

https://reviewboard.mozilla.org/r/89482/#review88746

> I'd prefer to reverse the logic of this bit if possible.  It seems like expanding auto-margins should be the default behavior.  Suppressing auto-margins here seems like a tweak for nsAbsoluteContainingBlock that other consumers shouldn't be penalized for.

Sure -- I've reversed it now. (calling it eIgnoreAutoMargins)

(I used "Expand" initially because the naming was easier to make super-clear. :)  "eExpandAutoMargins" is unambiguous -- but for the reversed-logic-scenario, I was imagining "eIgnoreAutoMargins" or "eCollapseAutoMargins", and I didn't really like either -- "ignore" is vague, "collapse" sounds too much like margin-collapsing.  Anyway, I've gone with "eIgnoreAutoMargins" now, and hopefully the flag's documentation [and the fact that it's only used in one place] will make its meaning clear enough.  If you can think of a better name, let me know.)
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 104

3 years ago
So the only thing that needs looking-at here is the updated Part 9:
  https://reviewboard.mozilla.org/r/89482/diff/

(The interdiff vs. the previous version isn't particularly interesting, since the logic has been reversed.)

Thanks!

Comment 105

3 years ago
mozreview-review
Comment on attachment 8805859 [details]
Bug 1269046 part 9: Add a flag to control whether CSSAlignUtils::AlignJustifySelf() expands "auto" margins.

https://reviewboard.mozilla.org/r/89482/#review88886

Looks good, thanks.
Attachment #8805859 - Flags: review?(mats) → review+

Comment 106

3 years ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/707a38d83b3b
part 1: Spin out some grid alignment code into a helper method, in a new CSSAlignUtils class. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/777060004f3f
part 2: Spin out a helper function to hold nsAbsoluteContainingBlock's code for resolving abspos offsets. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/85fa1048315a
part 3: Make a grid-specific comment more general, in CSSAlignUtils::AlignJustifySelf. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d5ca25a653b
part 4: Set flags on nsPlaceholderFrame & ReflowInput to track abspos frames that need CSS Box Alignment to resolve static position. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/6deb2d4c2f6b
part 5: If an abspos child's offset depends on CSS Box Alignment, ask nsContainerFrame for the alignment enum to use, and align with CSSAlignUtils. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/7978dd8d68ce
part 6: Add a flags enum-class to customize FlexboxAxisTracker behavior. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/b44098de2497
part 7: Give nsFlexContainerFrame a CSSAlignmentForAbsPosChild() implementation (to determine appropriate align enum for abspos children). r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/7085eed57189
part 8: Rewrite CSSAlignUtils::AlignJustifySelf to use a single "flags" arg instead of boolean args. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8965356a172
part 9: Add a flag to control whether CSSAlignUtils::AlignJustifySelf() expands "auto" margins. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c85b7c93c5e
part 10: Skip the normal margin-adding codepath for abspos frames that undergo CSS Box Alignment (which takes care of margins). r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/995211b23a55
part 11: Add reftests for CSS Alignment of absolutely positioned flex children.
Assignee

Updated

3 years ago
Depends on: 1313560

Updated

2 years ago
Depends on: 1330990
Assignee

Updated

2 years ago
Blocks: 1340309
Assignee

Updated

2 years ago
Depends on: 1343370
You need to log in before you can comment on or make changes to this bug.