Closed Bug 1312379 Opened 3 years ago Closed 3 years ago

[css-grid] inline-block baseline not computed correctly

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 + fixed
firefox53 --- fixed

People

(Reporter: jfernandez, Assigned: mats)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: testcase)

Attachments

(6 files, 3 obsolete files)

1.10 KB, text/html
Details
4.84 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
7.69 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
1.02 KB, text/html
Details
29.31 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
38.89 KB, text/plain
Details
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160921025640

Steps to reproduce:

1- load the attached test case.


Actual results:

The inline-block baseline is set as if it's grid/flex child has no baseline, hence using its border bottom edge. 


Expected results:

The inline-block baseline is set to its grid/flex child's baseline, as it happens with the case using block a box.

That's the current behavior using Chrome.
Product: Firefox → Core
Component: Untriaged → Layout
Version: 45 Branch → Trunk
Blocks: css-grid
Depends on: 1313068
Assignee: nobody → mats
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: testcase
OS: Unspecified → All
Hardware: Unspecified → All
Blocks: 1312403
Blocks: 1217086
The adds a few new baseline methods.  GetBaselineBOffset for getting the CSS Align
first/last baseline, or false if there is none.  GetVerticalAlignBaseline for getting a 'vertical-align' baseline, which basically just dispatches to the former, getting the last baseline for inline-blocks and the first baseline for everything else, and synthesizing a baseline when there is none.

There's still a bit more work to do to refactor the older methods; I'm planning
to remove nsLayoutUtils::GetFirst/LastLineBaseline etc, but I'm running out of
time for the Grid release.  Functionally this should work fine for now though.

Regarding the flexbox-align-self-baseline-horiz-3.xhtml test: I think this test
has some problems, even loading it in a currently Nightly on Linux shows
a difference between the test/reference.  It failed on OSX and Android,
but not other platforms on Try.  It seems to work fine if I comment out
the radio/checkbox form controls though.  I'll investigate more thoroughly
before landing, but I figured you could start reviewing before that.
Attachment #8816933 - Flags: review?(dholbert)
This just exports the flex containers last baseline when it exists.
I suspect that there are some pieces still missing in our Flexbox
implementation here - we should synthesize a baseline from the last
item in the last flex line when there is no last baseline group.
That is, bullet 2 in https://drafts.csswg.org/css-flexbox/#flex-baselines

I'm setting the container's last base to the first baseline in that case
for now, so that it should at least be correct for one-line flex containers.
It appears we have no reftests for this.
Attachment #8816934 - Flags: review?(dholbert)
Looks green on Try, https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8929a3f450df2d4bb5763aa5ef84f1977b86264&exclusion_profile=false
but I suspect there are a lot of combinations that we're not testing
at the moment.  I have a few manual tests that looks OK though.
I'll turn those into reftests...
So, I've debugged flexbox-align-self-baseline-horiz-3.xhtml failure
on OSX and this reduced testcase shows what's going on.  The checkbox
at the top is a flex item, the bottom one is in a block (the red line
is an indication of where the baseline is).

Currently these two cases render the same - we'll use the checkbox
content-box end edge as its baseline (see the special hack in
nsFormControlFrame::GetLogicalBaseline).  Note that the checkboxes
are rendered with a native theme here, which on OSX has a 2px border:
http://searchfox.org/mozilla-central/rev/3e157ac240611f80df409ae76221d46a31c20dfc/widget/cocoa/nsNativeThemeCocoa.mm#3009
so that explains why it fails on OSX (I assume the same reason
for the Android failure).

Now, this patchset makes us synthesize a baseline from the border-box
instead for the flex item, thus causing a difference between Flexbox
and Block (which still use GetLogicalBaseline).  This makes our checkbox
flex item layout compatible with Chrome, Safari and Edge though, so
I tend to think this is an improvement and that the reftest is bogus.
(I'll see if I rewrite the test a bit to reflect the new layout.)
Comment on attachment 8816933 [details] [diff] [review]
part 1 - Introduce nsIFrame methods for calculating baselines per CSS Alignment and CSS2 'vertical-align'.

Review of attachment 8816933 [details] [diff] [review]:
-----------------------------------------------------------------

Brief initial comments (sorry for not getting to this yet):

::: layout/generic/nsIFrame.h
@@ +1232,5 @@
> +   * https://drafts.csswg.org/css-align-3/#synthesize-baselines
> +   * For a first baseline the measure is from the border-box start edge and
> +   * for a last baseline the measure is from the border-box end edge.
> +   */
> +  nscoord SynthesizeBaselineFromMarginBox(mozilla::WritingMode aWM,

The documentation for this function should mention aWM.  Maybe add "with respect to the given writing-mode"?

(Some of our WritingMode-accepting APIs *require* the passed-in WritingMode to always be something in particular, whereas others are graceful and can handle any arbitrary writing-mode.  For public stuff like this, it's nice to document which category we're in.)

@@ +1234,5 @@
> +   * for a last baseline the measure is from the border-box end edge.
> +   */
> +  nscoord SynthesizeBaselineFromMarginBox(mozilla::WritingMode aWM,
> +                                          BaselineSharingGroup aGroup) const {
> +    auto margin = GetLogicalUsedMargin(aWM);

What follows this ^ is a pretty long function-body, for a method implemented in a .h file (particularly for a .h file which is #included all over the place).

Perhaps consider moving this implementation to nsFrame.cpp? (alongside the other nontrivial nsIFrame::Whatever() methods whose implementations live there)  I expect that'll help with code-size/compilation-time/readability-of-this-file.

@@ +1240,5 @@
> +      if (aWM.IsAlphabeticalBaseline()) {
> +        // First baseline for inverted line content is the block-start margin edge,
> +        // as the frame is in effect "flipped" for alignment purposes.
> +        return !aWM.IsLineInverted() ? -margin.BStart(aWM)
> +                                     : BSize(aWM) + margin.BEnd(aWM);

Is the logic here backwards? Right now the code looks like it's saying:
  if (line is not inverted) {
   return block-start margin edge (-margin.BStart);
  } else {
   return block-end margin edge;
  }

...which is the opposite of what the comments above it are saying. Maybe I'm misunderstanding though.

@@ +1241,5 @@
> +        // First baseline for inverted line content is the block-start margin edge,
> +        // as the frame is in effect "flipped" for alignment purposes.
> +        return !aWM.IsLineInverted() ? -margin.BStart(aWM)
> +                                     : BSize(aWM) + margin.BEnd(aWM);
> +      } else {

Drop the "else" here -- coding style guide forbids else-after-return.
(In reply to Daniel Holbert [:dholbert] from comment #6)
> > +  nscoord SynthesizeBaselineFromMarginBox(mozilla::WritingMode aWM,
> 
> The documentation for this function should mention aWM.  Maybe add "with
> respect to the given writing-mode"?

I've clarified the documentation.

> > +  nscoord SynthesizeBaselineFromMarginBox(mozilla::WritingMode aWM,
> > +                                          BaselineSharingGroup aGroup) const {
> 
> What follows this ^ is a pretty long function-body, for a method implemented
> in a .h file (particularly for a .h file which is #included all over the
> place).

I've moved the implementations to nsIFrameInlines.h for now.
I think it's important that they are inline for performance.
(typically, the BaselineSharingGroup is a constant so half the method
can be removed and the rest inlined.  We can consider separating them
to First/Last methods at a later point and/or moving them to nsFrame.h.
These methods are just a first rough cut of the API; I'm sure there
will be changes.)

> > +        return !aWM.IsLineInverted() ? -margin.BStart(aWM)
> > +                                     : BSize(aWM) + margin.BEnd(aWM);
> 
> Is the logic here backwards?

Oops, sorry about that.  Yes, there were a couple of bugs here
that cancelled each other out.

> Drop the "else" here -- coding style guide forbids else-after-return.

Fixed.

(also, I've fixed the flexbox-align-self-baseline-horiz-3.xhtml test now)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0a1392fed38a1e390e1735d15690446e13639b3&exclusion_profile=false
Attachment #8816933 - Attachment is obsolete: true
Attachment #8816933 - Flags: review?(dholbert)
Attachment #8818003 - Flags: review?(dholbert)
I was working a bit on bug 1321655 and I realized that there likely
is an issue w.r.t. to the aWM we give to these methods.
The IsAlphabeticalBaseline() and IsLineInverted() results are only valid
when the baseline axis is the frame's own inline-axis.
(I don't think we care about either when synthesizing a baseline in
the block-axis - we should use the "alphabetical" path in that case.)

So SynthesizeBaselineBOffsetFrom* are assuming the given aWM is
the frame's own WM.  The calls from FlexItem::ResolvedAscent and
nsLayoutUtils::GetFirstLinePosition is doing that.  The call from
nsIFrame::GetBaseline might not be though, depending on where its
aWM came from.  Both the calls from nsBlockFrame::GetLogicalBaseline
and nsGridContainerFrame::SynthesizeBaseline are using the child's
WM though, so I think we're OK, currently.  But the documentation
that says that you can use any old WM and get the correct results
is lying.  I'll update the comments again...  and use "aSelfWM"
instead to make it a bit clearer.

(I'll try to sort this out a bit better when dealing with orthogonal
items in bug 1321655.  Not for v52 though.)
I added a  MOZ_ASSERT(aWM == GetWritingMode()) to check the above, but found
that the call from nsBlockFrame::GetLogicalBaseline may have its aWM from here:
http://searchfox.org/mozilla-central/rev/5ee2bd8800b007d6c120d9521d5bf01131885afb/layout/generic/nsLineLayout.cpp#1440
which is the ancestor's WM.  Note that we know that the GetBlockDir()
of the WMs are the same there, which implies they are perpendicular.
The weaker MOZ_ASSERT(!aWM.IsOrthogonal(GetWritingMode())) holds though.

I suspect that aWM.IsLineInverted() doesn't really have any meaning unless
the alignment context is a block line. (As you can see, only
SynthesizeBaselineBOffsetFromMarginBox actually use that, which is only
used in inline contexts.)

The aWM.IsAlphabeticalBaseline() seems like it should use the alignment
context's WM though.  Specifically, it checks "!IsVertical() || IsSideways()"
http://searchfox.org/mozilla-central/rev/5ee2bd8800b007d6c120d9521d5bf01131885afb/layout/generic/WritingModes.h#286
So if we require aWM to be perpendicular to GetWritingMode() then
the IsVertical() part is the same, but the IsSideways() part may be
different.  So, I think Grid (and probably Flexbox) needs tweaking,
but I think that can go in a follow-up bug.  These patches should
at least improve the layout.

(The relevant spec seems to be this:
https://drafts.csswg.org/css-align-3/#baseline-export
"The alignment baseline is one of these, usually the dominant baseline
associated with the shared alignment context."
(the problem is 'dominant-baseline' only applies to "block containers
and inline boxes" but I assume the "dominant-baseline:auto" behavior
is what we should use for now:
https://www.w3.org/TR/css-inline-3/#dominant-baseline-property)
Writing-mode also says that the "parent’s dominant baseline" should be used:
 https://www.w3.org/TR/css-writing-modes-3/#baseline-alignment
Let me know if you find anything more specific in CSS Align/Grid/Flexbox)
Filed a spec issue for clarification of "alignment baseline is one of these":
https://github.com/w3c/csswg-drafts/issues/798
Clarified the comments some more.
Attachment #8818003 - Attachment is obsolete: true
Attachment #8818003 - Flags: review?(dholbert)
Attachment #8818148 - Flags: review?(dholbert)
I found a better spec reference - the Align spec is quite clear, here:
https://drafts.csswg.org/css-align-3/#align-by-baseline
"... the alignment baseline is the dominant baseline of the alignment context."
Comment on attachment 8818148 [details] [diff] [review]
part 1 - Introduce nsIFrame methods for calculating baselines per CSS Alignment and CSS2 'vertical-align'.

Review of attachment 8818148 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsBlockFrame.cpp
@@ +506,5 @@
> +    return nsLayoutUtils::GetFirstLineBaseline(aWM, this, aBaseline);
> +  }
> +
> +  for (ConstReverseLineIterator line = LinesRBegin(), line_end = LinesREnd();
> +       line != line_end; ++line) {

This loop (starting here) seems copypasted-with-edits from nsLayoutUtils::GetLastLineBaseline.

If that method is going to stick around, then you should probably include a comment here to explain why we can't use it (even though we *do* use its GetFirstLineBaseline analog, just above here).

Or alternately, if you think GetLastLineBaseline is eventually going away in favor of this method (not sure), perhaps file a followup on getting rid of it, and mark it as deprecated so people don't add new calls to it?

@@ +514,5 @@
> +      if (kid->GetVerticalAlignBaseline(aWM, &offset)) {
> +        // Ignore relative positioning for baseline calculations.
> +        const nsSize& sz = line->mContainerSize;
> +        offset += kid->GetLogicalNormalPosition(aWM, sz).B(aWM);
> +        *aBaseline = BSize(aWM) - offset;

This method seems to be assuming that aWM is the writing-mode of |this|, the nsBlockFrame -- in the call to BSize(aWM) here, as well as in the mixing of BSize(aWM) with line->BStart() below (where the line->BSize() call is implicitly using this nsBlockFrame's block/inline axis definitions).

BUT, per the documentation in nsIFrame::GetBaselineBOffset, aWM is really the writing-mode of "the alignment container" -- NOT the writing-mode of |this|.

Is that a problem?  (I think it is?)  Maybe we don't have any callsites right now that can trigger this, but it seems problematic -- and if it turns out that e.g. we really can assume that aWM matches our own writing-mode here, then we should include an assertion to that effect... (and probably hint at why that is, since that seems to go against the documentation)

::: layout/generic/nsGridContainerFrame.h
@@ +125,5 @@
> +    if (HasAnyStateBits(NS_STATE_GRID_SYNTHESIZE_BASELINE)) {
> +      return false;
> +    }
> +    GetBBaseline(aBaselineGroup, aBaseline);
> +    return true;

GetBBaseline actually returns type 'bool' [and could conceivably fail without this callsite noticing -- though it never does].


So: for robustness & conciseness, perhaps the last 2 lines here should be collapsed together into:
  return GetBBaseline(aBaselineGroup, aBaseline);

::: layout/generic/nsIFrame.h
@@ +425,5 @@
> +  eFirst = 0,
> +  eLast = 1,
> +};
> +
> +// Loosely: https://www.w3.org/TR/css-align-3/#shared-alignment-context

Let's change this to link to the editor's draft, rather than the TR, for consistency & latest-and-greatest.

(All the other spec links in this patch are pointing at the ED, with the "drafts.csswg.org" domain -- this is the only code-comment with a TR link.)

@@ +1229,5 @@
> +
> +  /**
> +   * Synthesize a first(last) inline-axis baseline from our margin-box.
> +   * An alphabetical baseline is at the start(end) edge and a central baseline
> +   * is at the center of our block-axis margin-box (aWM tells which to use).

For an abundance of clarity, you might consider tweaking the second-to-last line here like so:
  s/start(end) edge/start(end) edge of our block-axis margin-box,/

(It's easy to get confused about which dimension of the margin box we're talking about, particularly since the first line mentions "inline-axis".)

@@ +1231,5 @@
> +   * Synthesize a first(last) inline-axis baseline from our margin-box.
> +   * An alphabetical baseline is at the start(end) edge and a central baseline
> +   * is at the center of our block-axis margin-box (aWM tells which to use).
> +   * https://drafts.csswg.org/css-align-3/#synthesize-baselines
> +   * @note You should only call this on frames that are perpendicular to aWM.

I'm not sure what it means for a frame to "be perpendicular to aWM". Could you clarify? (I don't think it's well-defined what it means for a *frame* to be perpendicular to a writing mode.  My first assumption was that this meant to say "frames whose GetWritingMode() is orthogonal [i.e. perpendicular] to aWM".  But in fact it looks like these methods all assert the exact opposite -- they assert that GetWritingMode() is NOT orthogonal to aWM. So I think the language might be backwards here, too?)

Perhaps this really wants to say: "on frames whose GetWritingMode() is parallel to aWM"? (notably saying "parallel", not perpendicular)  Also, be sure to fix all the instances of this @note -- I think there are 6.

@@ +1234,5 @@
> +   * https://drafts.csswg.org/css-align-3/#synthesize-baselines
> +   * @note You should only call this on frames that are perpendicular to aWM.
> +   * @param aWM the writing-mode of the alignment container
> +   * @return an offset from our border-box block-axis start(end) edge for
> +   * for a first(last) baseline respectively

Nit: s/for for/for/

(it's repeated across the linebreak here)

@@ +1253,5 @@
> +   * @return an offset from our border-box block-axis start(end) edge for
> +   * for a first(last) baseline respectively
> +   * (implemented in nsIFrameInlines.h)
> +   */
> +  inline nscoord SynthesizeBaselineBOffsetFromBorderBox(

The three documentation-nits for the method before this one all apply here as well. (RE "edge", "perpendicular", & "for for")

@@ +1275,5 @@
> +
> +  /**
> +   * Return true if the frame has a CSS2 'vertical-align' baseline.
> +   * If it has, then the returned baseline is the distance from the block-
> +   * axis border-box start edge.

I'm unclear on the semantic difference between this and GetLogicalBaseline (besides the fact that this one returns bool + outparam). Their documentation sounds similar, and their impls are similar as well -- e.g. nsBlockFrame's implementations of these two methods are nearly the same, it looks like, and nsGfxScrollFrame and nsFlexContainerFrame both basically have one of the methods call the other one. (though nsFlexContainerFrame has a state-bit check and one level of indirection)

Could you clarify the documentation to make it clearer what the connection is between these methods, and how to know which of the two should be used in a particular context?

@@ +1280,5 @@
> +   * @note The returned value is only valid when reflow is not needed.
> +   * @note You should only call this on frames that are perpendicular to aWM.
> +   * @param aWM the writing-mode of the alignment container
> +   * @param aBaseline the baseline offset, only valid if the method returns true
> +   * @return true if the frame has an inline-axis baseline

The @return line here is wrong (copypaste typo, from copying GetBaseline's documentation).

This documentation here is for GetVerticalAlignBaseline, so you probably meant for this line to say something like:
   * @return true if the frame has a CSS2 'vertical-align' baseline

@@ +1289,5 @@
> +  }
> +
> +  /**
> +   * Return true if the frame has a first(last) inline-axis baseline per
> +   * CSS Alignment.  If it has, then the returned baseline is the distance from

Two things:
 (1) s/CSS Alignment/CSS Box Alignment/ (inserting "Box")
 (2) It's not clear what "per CSS [Box] Alignment" means here. (In particular: I don't know whether it's the offset *for use in CSS Box Alignment*, or the offset *determined by performing CSS Box alignment on our children*, or something else.)

@@ +1295,5 @@
> +   * @note The returned value is only valid when reflow is not needed.
> +   * @note You should only call this on frames that are perpendicular to aWM.
> +   * @param aWM the writing-mode of the alignment container
> +   * @param aBaseline the baseline offset, only valid if the method returns true
> +   * @return true if the frame has an inline-axis baseline

As above, copypaste typo on the @return line here. (This one should mention something about css box alignment, presumably.)

@@ +1297,5 @@
> +   * @param aWM the writing-mode of the alignment container
> +   * @param aBaseline the baseline offset, only valid if the method returns true
> +   * @return true if the frame has an inline-axis baseline
> +   */
> +  virtual bool GetBaselineBOffset(mozilla::WritingMode aWM,

This name "GetBaselineBOffset()" seems too easy to confuse with "GetBaseline()" (which also happens to return a B-offset -- hence, easy to confuse GetBaseline with GetBaselineBOffset).

Maybe "GetAlignBaselineOffset" would be clearer, since this is for CSS Box Alignment? *shrug*

::: layout/generic/nsIFrameInlines.h
@@ +104,5 @@
> +  MOZ_ASSERT(!aWM.IsOrthogonalTo(GetWritingMode()));
> +  auto margin = GetLogicalUsedMargin(aWM);
> +  if (aGroup == BaselineSharingGroup::eFirst) {
> +    if (aWM.IsAlphabeticalBaseline()) {
> +      // First baseline for inverted line content is the block-start margin edge,

Consider hyphenating "inverted-line" to make the grouping clearer here.

(This is meant to be read "[inverted line] content", rather than "inverted [line content]")

This happens twice.

@@ +109,5 @@
> +      // as the frame is in effect "flipped" for alignment purposes.
> +      return MOZ_UNLIKELY(aWM.IsLineInverted()) ? -margin.BStart(aWM)
> +                                                : BSize(aWM) + margin.BEnd(aWM);
> +    }
> +    nscoord marginBoxCenter = (BSize(aWM) + margin.BStartEnd(aWM)) / 2;

Maybe name this "marginBoxHalfSize"?   This is a size, whereas "center" sounds more like a position (and this discrepancy causes some confusion in the arithmetic after this, where we derive a position from this value).

@@ +112,5 @@
> +    }
> +    nscoord marginBoxCenter = (BSize(aWM) + margin.BStartEnd(aWM)) / 2;
> +    return marginBoxCenter -
> +           (MOZ_UNLIKELY(aWM.IsLineInverted()) ? margin.BStart(aWM)
> +                                               : margin.BEnd(aWM));

Two things:
 (1) I don't think margin.BEnd(aWM) makes sense in the arithmetic here... I think this might be a mistake?
 (2) I don't think you need a ternary statement (i.e. I don't think you need to condition on IsLineInverted here), though I'm not 100% sure.

Expanding on (1): Right now, in the "likely" (i.e. not-inverted) case, you're returning:
  marginBoxHalfSize - margin.BEnd(aWM)

What you *want* to be returning is: the offset to the margin-box center position, measured from the border-box start edge.  I believe that offset should *really* be computed like so:
  [position of margin-box-start edge, w.r.t. border-box-start edge] + [half of margin-box-size]

So in code, that should be:
  -margin.BStart(aWM) + marginBoxHalfSize

So we should be subtracting BStart, not BEnd, in this "likely" case. It seems pretty clear to me that that's the correct return-value for the not-inverted case.

Expanding on (2): For the inverted case, I *think* it's correct to subtract BStart as well, since we're still measuring from the same position (the border-start edge) to the same place (the halfway point of the margin)... So you might not need a ternary statement at all here.

@@ +126,5 @@
> +  nscoord marginBoxSize = BSize(aWM) + margin.BStartEnd(aWM);
> +  nscoord marginBoxCenter = (marginBoxSize / 2) + (marginBoxSize % 2);
> +  return marginBoxCenter -
> +         (MOZ_UNLIKELY(aWM.IsLineInverted()) ? margin.BEnd(aWM)
> +                                             : margin.BStart(aWM));

As above:
 * s/marginBoxCenter/marginBoxHalfSize/
 * I don't understand the BEnd / BStart subtraction distinction here, nor do I understand why we're checking IsLineInverted.  In this case, I'd expect you'd want "-margin.BEnd(aWM) + marginBoxHalfSize" for the "likely" case, and probably for the unlikely (line-inverted) case as well.

@@ +142,5 @@
> +                                                    : borderBoxSize / 2;
> +  }
> +  MOZ_ASSERT(aGroup == BaselineSharingGroup::eLast);
> +  // Round up for central baseline offset, to be consistent with eFirst.
> +  auto borderBoxCenter = (borderBoxSize / 2) + (borderBoxSize % 2);

(Perhaps s/borderBoxCenter/borderBoxHalfSize/, for consistency with my marginBoxHalfSize suggestion for the analogous code above.)
Comment on attachment 8816934 [details] [diff] [review]
part 2 - [css-flexbox] Improve support for CSS Alignment 'last baseline' alignment by exporting the last baseline when asked for.

Review of attachment 8816934 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on part 2, with one nit addressed:

::: layout/generic/nsFlexContainerFrame.h
@@ +96,5 @@
>      if (HasAnyStateBits(NS_STATE_FLEX_SYNTHESIZE_BASELINE)) {
>        return false;
>      }
> +    *aBaseline = aBaselineGroup == BaselineSharingGroup::eFirst ?
> +                   GetLogicalBaseline(aWM) : mLastBaselineFromLastReflow;

Is there a reason we need to call GetLogicalBaseline(aWM) in the eFirst case?  I think we can & should just return the cached mBaselineFromLastReflow.  (That would make this ternary condition a little more symmetrical, too.)

In particular -- when NS_STATE_FLEX_SYNTHESIZE_BASELINE is unset (as it must be, when we get to this point in the code), then nsFlexContainerFrame::GetLogicalBaseline() unconditionally returns mBaselineFromLastReflow.  So we might as well just directly return that variable here.
Attachment #8816934 - Flags: review?(dholbert) → review+
Comment on attachment 8816935 [details] [diff] [review]
part 3 - [css-align][css-tables] Add methods for CSS Alignment first/last baseline of the table container.

Review of attachment 8816935 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on part 3
Attachment #8816935 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] (PTO thru 12/18) from comment #13)
> This loop (starting here) seems copypasted-with-edits from
> nsLayoutUtils::GetLastLineBaseline.
> 
> If that method is going to stick around, then you should probably include a
> comment here to explain why we can't use it (even though we *do* use its
> GetFirstLineBaseline analog, just above here).

I'm planning to remove both nsLayoutUtils::GetFirst/LastLineBaseline
eventually.

> Or alternately, if you think GetLastLineBaseline is eventually going away in
> favor of this method (not sure), perhaps file a followup on getting rid of
> it, and mark it as deprecated so people don't add new calls to it?

Good point, but the new API here is still WIP and expected to change
so I think it's a bit too early to say the old ones are deprecated.

> > +        *aBaseline = BSize(aWM) - offset;
> 
> This method seems to be assuming that aWM is the writing-mode of |this|, the
> nsBlockFrame -- in the call to BSize(aWM) here, as well as in the mixing of
> BSize(aWM) with line->BStart() below (where the line->BSize() call is
> implicitly using this nsBlockFrame's block/inline axis definitions).

This code is just copied from nsLayoutUtils::GetLastLineBaseline though:
https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/layout/base/nsLayoutUtils.cpp#5977-5978
nsBlockFrame::GetLogicalBaseline currently just forwards its aWritingMode:
https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/layout/generic/nsBlockFrame.cpp#496
so I guess any bug already exists there?

But you're right, it does look suspicious.  It seems like an existing
issue though, so I think we can sort this out in a follow-up bug?

> ::: layout/generic/nsGridContainerFrame.h
> So: for robustness & conciseness, perhaps the last 2 lines here should be
> collapsed together into:
>   return GetBBaseline(aBaselineGroup, aBaseline);

Fixed.

> ::: layout/generic/nsIFrame.h
> > +// Loosely: https://www.w3.org/TR/css-align-3/#shared-alignment-context
> 
> Let's change this to link to the editor's draft, rather than the TR, for
> consistency & latest-and-greatest.

Yeah, I must have followed an external link from some other spec and
ended up on a TR without noticing.

> > +   * Synthesize a first(last) inline-axis baseline from our margin-box.
> > +   * An alphabetical baseline is at the start(end) edge and a central baseline
> > +   * is at the center of our block-axis margin-box (aWM tells which to use).
> 
> For an abundance of clarity, you might consider tweaking the second-to-last
> line here like so:
>   s/start(end) edge/start(end) edge of our block-axis margin-box,/

But the sentence already states "of our block-axis margin-box" so why
do you think it's ambigious?

> I'm not sure what it means for a frame to "be perpendicular to aWM". 

Sorry, I used the wrong word there - I meant parallel.

> I'm unclear on the semantic difference between this and GetLogicalBaseline
> (besides the fact that this one returns bool + outparam). Their
> documentation sounds similar, and their impls are similar as well -- e.g.
> nsBlockFrame's implementations of these two methods are nearly the same, it
> looks like, and nsGfxScrollFrame and nsFlexContainerFrame both basically
> have one of the methods call the other one. (though nsFlexContainerFrame has
> a state-bit check and one level of indirection)

GetVerticalAlignBaseline is meant to replace GetLogicalBaseline eventually,
with the difference that it returns false when there is no natural baseline
to indicate the caller should synthesize one.  The name is meant to indicate
it's only to be used for vertical-align:baseline, as opposed to CSS Align.
It returns whichever baseline should be used for aligning the box on
a line, based on the box type - last-baseline for blocks and first-baseline
for grid/flexbox containers etc, per CSS2.

>  (2) It's not clear what "per CSS [Box] Alignment" means here.

If it has a 'natural baseline' per CSS [Box] Alignment.
https://drafts.csswg.org/css-align-3/#natural-baseline

> > +   * @return true if the frame has an inline-axis baseline
> 
> As above, copypaste typo on the @return line here. (This one should mention
> something about css box alignment, presumably.)

I removed the @return notes for the methods that are documented as
"Return [description]" since that description is more elaborate.

> > +  virtual bool GetBaselineBOffset(mozilla::WritingMode aWM,
> 
> Maybe "GetAlignBaselineOffset" would be clearer, since this is for CSS Box
> Alignment? *shrug*

I guess I could rename it GetNaturalBaselineBOffset, but I'm concerned
about very long method names because of our archaic 80-column rule.
I think the description of GetBaseline() is pretty clear that it returns
the natural baseline (when one exists) or a synthesized baseline if not.

> ::: layout/generic/nsIFrameInlines.h
> > +    nscoord marginBoxCenter = (BSize(aWM) + margin.BStartEnd(aWM)) / 2;
> 
> Maybe name this "marginBoxHalfSize"?   This is a size, whereas "center"
> sounds more like a position (and this discrepancy causes some confusion in
> the arithmetic after this, where we derive a position from this value).

No, it actually is a position - the center of the margin-box (as measured
from the start of the margin-box).  Then we convert this position to be
a position from the start of the border-box.  (Also, I think "center"
makes the intent clearer than "halfsize".)

> Two things:
>  (1) I don't think margin.BEnd(aWM) makes sense in the arithmetic here... I
> think this might be a mistake?
>  (2) I don't think you need a ternary statement (i.e. I don't think you need
> to condition on IsLineInverted here), though I'm not 100% sure.

OK, I've removed the IsLineInverted stuff for now, and swapped
the BStart/BEnd.
Attachment #8818148 - Attachment is obsolete: true
Attachment #8818148 - Flags: review?(dholbert)
Attachment #8818775 - Flags: review?(dholbert)
(In reply to Daniel Holbert [:dholbert] (PTO thru 12/18) from comment #14)
> I think we can & should just return the cached mBaselineFromLastReflow.

OK, will do.
(In reply to Mats Palmgren (:mats) from comment #16)
Note: I've flagged my requests with REQUEST:, so they're easier to spot via find-in-page. needinfo=you for thoughts/action on those.
> This code is just copied from nsLayoutUtils::GetLastLineBaseline though:
> https://dxr.mozilla.org/mozilla-central/rev/
> 8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/layout/base/nsLayoutUtils.cpp#5977-
> 5978
> nsBlockFrame::GetLogicalBaseline currently just forwards its aWritingMode:
> https://dxr.mozilla.org/mozilla-central/rev/
> 8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/layout/generic/nsBlockFrame.cpp#496
> so I guess any bug already exists there?
> 
> But you're right, it does look suspicious.  It seems like an existing
> issue though, so I think we can sort this out in a follow-up bug?

I'm not sure if the bug already exists... This is *becoming* suspicious in this patch because we're adding documentation here that requires aWM to be the alignment container's WM. Not sure if that requirement exists in the current location (or how thoroughly we require it at the new location).

Anyway, a followup is OK I suppose. (REQUEST: please file that :))

> > > +   * Synthesize a first(last) inline-axis baseline from our margin-box.
> > > +   * An alphabetical baseline is at the start(end) edge and a central baseline
> > > +   * is at the center of our block-axis margin-box (aWM tells which to use).
> > 
> > For an abundance of clarity, you might consider tweaking the second-to-last
> > line [...]
> But the sentence already states "of our block-axis margin-box" so why
> do you think it's ambigious?

No need to change it, I suppose -- but (to answer your question): the way I read it, that "margin-box" qualifier is part of a *different* (and special-casey) case -- the central-baseline casee. The current wording is ambiguous about whether that qualifier also applies to the (earlier) alphabetic baseline wording.  And when I read the current alphabetic-baseline wording, it sounds like we might be talking about an "inline-axis"-flavored measurement (since that's the last axis that was mentioned).

*shrug*, it's fine as-is. :)

> > I'm unclear on the semantic difference between this and GetLogicalBaseline
> [...]
> GetVerticalAlignBaseline is meant to replace GetLogicalBaseline eventually

REQUEST: Could you add a comment to that effect in either or both methods' documentation?  That makes the distinction (or lack thereof) make more sense -- and it makes it easier to figure out which one to use at new callsites (probably GetVerticalAlignBaseline is best).

> I removed the @return notes for the methods that are documented as
> "Return [description]" since that description is more elaborate.

Fine by me - yeah, no sense in repeating that info twice in the same comment. Thanks!

> > > +  virtual bool GetBaselineBOffset(mozilla::WritingMode aWM,
> > 
> > Maybe "GetAlignBaselineOffset" would be clearer, since this is for CSS Box
> > Alignment? *shrug*
> 
> I guess I could rename it GetNaturalBaselineBOffset, but I'm concerned
> about very long method names because of our archaic 80-column rule.

REQUEST: How about just renaming it to "GetNaturalBaseline" then?  That's about as long as the current method name, and it's much clearer IMO.  (You don't bother with "BOffset" in the naming of any of the other baseline methods, even though they all return a "B Offset" as well -- so if you're concerned with brevity, I think "Natural" is a more useful/distinguishing thing to use in the method name, rather than "BOffset".)

> > ::: layout/generic/nsIFrameInlines.h
> > > +    nscoord marginBoxCenter = (BSize(aWM) + margin.BStartEnd(aWM)) / 2;
> > 
> > Maybe name this "marginBoxHalfSize"?   This is a size, whereas "center"
> > sounds more like a position (and this discrepancy causes some confusion in
> > the arithmetic after this, where we derive a position from this value).
> 
> No, it actually is a position - the center of the margin-box (as measured
> from the start of the margin-box).  Then we convert this position to be
> a position from the start of the border-box.  (Also, I think "center"
> makes the intent clearer than "halfsize".)

Sorry -- I didn't make my concern very clear. The reason "marginBoxCenter" (as a position) is confusing is that the "margin box center" is in fact *the very position that we're trying to compute and return*.  So "return [complex expression that involves marginBoxCenter]" is kinda confusing, because marginBoxCenter already sounds like the thing we should be returning in the first place.

It's less confusing now that there's no ternary expression involved in that return statement [it's more obvious that we're just changing coordinate spaces], so I think this is fine now.
Flags: needinfo?(mats)
Comment on attachment 8818775 [details] [diff] [review]
part 1 - Introduce nsIFrame methods for calculating baselines per CSS Alignment and CSS2 'vertical-align'.

Review of attachment 8818775 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with REQUESTs in comment 18 addressed as you see fit, and with this nit:

::: layout/reftests/flexbox/flexbox-align-self-baseline-horiz-3-ref.xhtml
@@ +27,5 @@
>        .pink   { background: pink;   }
>        .aqua   { background: aqua;   }
> +
> +      i { display:inline-block; width:20px; height:2px; background:black; }
> +      .ref { 

nit: drop end-of-line space character
Attachment #8818775 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #18)
> Anyway, a followup is OK I suppose. (REQUEST: please file that :))

Bug 1324321.  I've documented it like this for now:
>   * @param aWM the writing-mode of the alignment context, with the ltr/rtl
>   * direction tweak done by nsIFrame::GetWritingMode(nsIFrame*) in inline
>   * contexts (see that method).


> > > I'm unclear on the semantic difference between this and GetLogicalBaseline
> > [...]
> > GetVerticalAlignBaseline is meant to replace GetLogicalBaseline eventually
> 
> REQUEST: Could you add a comment to that effect in either or both methods'
> documentation?

Done.

> REQUEST: How about just renaming it to "GetNaturalBaseline" then?

BOffset is nice to have since it follows an established pattern that
makes it easier to spot errors that mixes B and I measures.

I renamed it GetNaturalBaselineBOffset, because when it comes to
a choice of clarity vs the 80-column rule, the former wins.
I also renamed GetBaseline() to BaselineBOffset() for clarity.
Flags: needinfo?(mats)
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/950dba8823a0
part 1 - [css-align][css-flexbox][css-grid] Introduce nsIFrame methods for calculating baselines per CSS Alignment and CSS2 'vertical-align'.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/45ec6acc712f
part 2 - [css-flexbox] Improve support for CSS Alignment 'last baseline' alignment by exporting the last baseline when asked for.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/15cdeb891c97
part 3 - [css-align][css-tables] Add methods for CSS Alignment first/last baseline of the table container.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/efcaf80ef859
part 4 - Rename GetBaseline() to BaselineBOffset().  r=me
Flags: in-testsuite+
Comment on attachment 8818775 [details] [diff] [review]
part 1 - Introduce nsIFrame methods for calculating baselines per CSS Alignment and CSS2 'vertical-align'.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: baseline alignment not working for grid/flex containers
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: bug 1313811 and bug 1313068 
[Is the change risky?]:no
[Why is the change risky/not risky?]: it mostly affects grid/flexbox which have fairly good test coverage
[String changes made/needed]:none

This is part of the CSS Grid release, please uplift to Aurora 52, thanks.

It's probably best if you uplift them in the same order as they landed:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=efcaf80ef8590b3dc41b75e836cd13f308a413a1
Attachment #8818775 - Flags: approval-mozilla-aurora?
[Tracking Requested - why for this release]: needed for CSS Grid release
track css grid issue for 52
Comment on attachment 8818775 [details] [diff] [review]
part 1 - Introduce nsIFrame methods for calculating baselines per CSS Alignment and CSS2 'vertical-align'.

take new css-grid patch set for aurora52
Attachment #8818775 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Mats Palmgren (:mats) from comment #23)
> Comment on attachment 8818775 [details] [diff] [review]
> part 1 - Introduce nsIFrame methods for calculating baselines per CSS
> Alignment and CSS2 'vertical-align'.
> 
> Approval Request Comment
> [Feature/Bug causing the regression]:
> [User impact if declined]: baseline alignment not working for grid/flex
> containers
> [Is this code covered by automated tests?]:yes
> [Has the fix been verified in Nightly?]:yes
> [Needs manual test from QE? If yes, steps to reproduce]: no
> [List of other uplifts needed for the feature/fix]: bug 1313811 and bug
> 1313068 
> [Is the change risky?]:no
> [Why is the change risky/not risky?]: it mostly affects grid/flexbox which
> have fairly good test coverage
> [String changes made/needed]:none
> 
> This is part of the CSS Grid release, please uplift to Aurora 52, thanks.
> 
> It's probably best if you uplift them in the same order as they landed:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=efcaf80ef8590b3dc41b75e836cd13f308a413a1

mats: bug 1313068 has problems to apply so leaving this out of the uplift for now
needs as well rebasing 

grafting 380516:950dba8823a0 "Bug 1312379 part 1 - [css-align][css-flexbox][css-grid] Introduce nsIFrame methods for calculating baselines per CSS Alignment and CSS2 'vertical-align'.  r=dholbert"
merging layout/base/nsLayoutUtils.cpp
merging layout/generic/nsBlockFrame.cpp
merging layout/generic/nsFlexContainerFrame.cpp
merging layout/generic/nsFlexContainerFrame.h
merging layout/generic/nsGfxScrollFrame.h
merging layout/generic/nsGridContainerFrame.cpp
merging layout/generic/nsGridContainerFrame.h
merging layout/generic/nsIFrame.h
merging layout/reftests/flexbox/reftest.list
warning: conflicts while merging layout/base/nsLayoutUtils.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging layout/generic/nsFlexContainerFrame.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(mats)
Backed out for permafailing flexbox-align-self-baseline-horiz-3.xhtml on OSX:

Push containing failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=af6633c41d46a6183d9173cd6aa13532ed6a733b
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=4546076&repo=mozilla-aurora

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/flexbox/flexbox-align-self-baseline-horiz-3.xhtml == file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/flexbox/flexbox-align-self-baseline-horiz-3-ref.xhtml | image comparison, max difference: 243, number of differing pixels: 116
Hmm, that's odd.  Comparing flexbox-align-self-baseline-horiz-3*.xhtml with the Aurora patches
applied shows that they are identical to trunk.  So there must be some other change they
depend on.  Looking at the layout/forms/nsGfxRadioControlFrame.cpp change log, I'm guessing
it's bug 418833.  The error is in the -ref rendering which is using -moz-appearance:none,
and if I squint, it does look like it got the baseline right.

I think I'll just comment out this test on Aurora and reland.  It's just testing checkbox
baseline alignment in Flexbox so it's not super-important, and it seems very unlikely we'll
regress that bit without discovering it on trunk first (which will continue to run the test).
Flags: needinfo?(mats)
Looks like Pulsebot is gone for the holidays too... :-)
Oh, and the Bugherder menu entry to get a list of changeset links is gone.
Instead, it wants an "API key" to be able to submit that to Bugzilla.
Sigh.
Well, here's the Aurora revisions anyway:
20654fa125a9
81c33b6cf450
99c2c958f3d2
93ed33a01abe
Yeah, the summary page won't show anything until you actually submit the changes to bugzilla. 

On the earlier page on Bugherder, you can click each revision's "View Comment" link to show the link to the commit, which is slower than the summary page would be, but not truly horrible for only marking four commits:

https://hg.mozilla.org/releases/mozilla-aurora/rev/20654fa125a9
https://hg.mozilla.org/releases/mozilla-aurora/rev/81c33b6cf450
https://hg.mozilla.org/releases/mozilla-aurora/rev/99c2c958f3d2
https://hg.mozilla.org/releases/mozilla-aurora/rev/93ed33a01abe
(Amusingly, the reftest failures continued, even on the push with the backouts, so I'd suspect one of the other patches in your original push was the cause for the failures. But they're gone now that the test is being skipped.)
Depends on: 1335552
You need to log in before you can comment on or make changes to this bug.