1313068 - [css-grid] grid uses item's margin-box to synthesize a baseline

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Javier Fernandez]

Attachment: baseline-synthesize-bug.html

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 grid's baseline is synthesized using item's margin-box.


Expected results:

The grid's baseline should be synthesized using item's border box.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Synthesize a grid container baseline from the margin-box when in an inline-level context, and from the border-box otherwise.

This is a bit wallpaper-ish, but it'll do for now.  I'll continue working on
baselines in bug 1312379, which should make this better.

Comment 2

[Mats Palmgren (inactive)]

Attachment: reftests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38d45c6ae7a8dc96c8652aaabd01cca686b845e1&exclusion_profile=false

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8805818 [details] [diff] [review]
Synthesize a grid container baseline from the margin-box when in an inline-level context, and from the border-box otherwise.

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

::: layout/base/nsLayoutUtils.cpp
@@ +6179,5 @@
> +           aFrame->HasAnyStateBits(NS_STATE_GRID_SYNTHESIZE_BASELINE)) ||
> +          (fType == nsGkAtoms::flexContainerFrame &&
> +           aFrame->HasAnyStateBits(NS_STATE_FLEX_SYNTHESIZE_BASELINE)) ||
> +          (fType == nsGkAtoms::tableWrapperFrame &&
> +           static_cast<const nsTableWrapperFrame*>(aFrame)->GetRowCount() == 0)) {

(Is GetRowCount() really all we need to check here, for tables?  What if we have an empty table which has a caption -- should we be falling back on that for the baseline? If we're not sure, perhaps it's better to punt on tables for now?)

::: layout/generic/nsFrame.h
@@ +642,5 @@
> +   * this is the end edge of the border box.  For a central baseline it's
> +   * the center of the border box.
> +   * https://drafts.csswg.org/css-align-3/#synthesize-baselines
> +   */
> +  nscoord SynthesizeBaselineFromBorderBox(mozilla::WritingMode aWM) const

Is there a reason you're putting this on nsFrame instead of nsIFrame?

If it were on nsIFrame, we could avoid the static_cast in nsLayoutUtils.cpp, which would be great...

::: layout/generic/nsFrameStateBits.h
@@ +307,5 @@
>  // (Means that we have to be more thorough about checking them for sortedness.)
>  FRAME_STATE_BIT(FlexContainer, 20, NS_STATE_FLEX_CHILDREN_REORDERED)
>  
> +// True if the container has no flex items; may lie if there is a pending reflow
> +FRAME_STATE_BIT(GridContainer, 21, NS_STATE_FLEX_SYNTHESIZE_BASELINE)

Copypaste typo -- this needs s/GridContainer/FlexContainer/

Also, the comment kind of lies, because we don't actually set this anywhere yet. Please file a followup and tag it in an XXX comment here? (so that the comment can continue making its optimistic statement, and then the XXX comment can walk it back a little)

::: layout/generic/nsGridContainerFrame.cpp
@@ +6064,5 @@
>    } else {
>      RemoveStateBits(NS_STATE_GRID_NORMAL_FLOW_CHILDREN_IN_CSS_ORDER);
>    }
> +  if (gridReflowInput.mIter.AtEnd()) {
> +    // No grid items means we should synthesize a baseline if needed.

The phrase "No grid items means..." is a bit hard to read. (It sounds like it's saying "None of our grid items mean ...".)

Instead, how about just:
    // We have no grid items, so we should synthesize a baseline if needed.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8805818 [details] [diff] [review]
Synthesize a grid container baseline from the margin-box when in an inline-level context, and from the border-box otherwise.

r- for now, for the first two questions in comment 3.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Expected rendering for the baseline-synthesize-bug.html testcase

This is the rendering I have in my local build at the moment.
I believe it's the correct one per spec(s), so there is a small error
in the testcase.

The first grid on the line has a grid item and derives its baseline from
that item, that (block) item however doesn't have any lines, so a baseline
is synthesized for the item.  For grid items, the baseline is synthesized
from the *border-box*, so the first grid container's baseline is at the
bottom edge of the pink border, this also becomes the baseline for the line
in the inline-block that wraps the grid and thus the baseline for the
inline-block itself.

The second grid on the line has no grid items and thus a baseline is
synthesized for the *grid container*.  This grid container is in a
block context and thus a baseline is synthesized from its *margin-box*.
There is no inline-block wrapper in this case so the end edge of
the margin-box is aligned to the line.

IOW, the different contexts for synthesizing the baselines: grid context
use the border-box, block context use the margin-box is what makes
the grey squares not align correctly.  The difference is the 8px
margin-bottom on the second grid.

(The flexbox testcase is actually exactly the same, it's just that
the 'background-color:lightgrey' was omitted for the flexbox containers
(by mistake I assume).  Adding it makes the grid/flexbox tests look
the same.)

Javier, do you agree with the above analysis and that this is
the correct rendering?

Comment 6

[Javier Fernandez]

(In reply to Mats Palmgren (:mats) from comment #5)
> Created attachment 8815952 [details]
> Expected rendering for the baseline-synthesize-bug.html testcase
> 
> This is the rendering I have in my local build at the moment.
> I believe it's the correct one per spec(s), so there is a small error
> in the testcase.
> 

Umm, I'm not sure what do you mean with a "smal error"; it's true that
flexbox does not have the lightgrey background, which can be confusing.
Perhaps the text "should align with the middle of the grey box" is also
confusing, but that test case was extracted from one of the Chrome's 
flexbox implementation, which is what actually does; I think this is a
bug, so I filed https://crbug.com/659610 to track this issue.

Anyway, the rendering you show is different to the one I've got in my
current FF build, so there was definitively some fixes. I'll comment 
about the different cases below.

> The first grid on the line has a grid item and derives its baseline from
> that item, that (block) item however doesn't have any lines, so a baseline
> is synthesized for the item.  For grid items, the baseline is synthesized
> from the *border-box*, so the first grid container's baseline is at the
> bottom edge of the pink border, this also becomes the baseline for the line
> in the inline-block that wraps the grid and thus the baseline for the
> inline-block itself.
> 

I agree with this case and that's what I recently implemented in Chrome.

> The second grid on the line has no grid items and thus a baseline is
> synthesized for the *grid container*.  This grid container is in a
> block context and thus a baseline is synthesized from its *margin-box*.
> There is no inline-block wrapper in this case so the end edge of
> the margin-box is aligned to the line.

I could agree on this case as well. This is what I asked for in the
CSS WG github issue (https://github.com/w3c/csswg-drafts/issues/638) and
fantasai confirmed that we should use the margin-box. As far as I understand,
that's what the Writing Modes spec states on this regard:

 https://www.w3.org/TR/css-writing-modes-3/#replaced-baselines

However, currently Flexbox uses the container's content-box as synthesized
baseline when in an inline-level context. The second grid is indeed an
inline-block and it's part of the inline-block used for rendering the text
line.

I decided, for now, to emulate this behavior in grid, but sincerely, I couldn't
find out where in the spec it's stated such behavior.

> 
> IOW, the different contexts for synthesizing the baselines: grid context
> use the border-box, block context use the margin-box is what makes
> the grey squares not align correctly.  The difference is the 8px
> margin-bottom on the second grid.

Yes, I agree the boxes should align differently. This bug is not to
fix that behavior. My FF local build uses item's margin box in the first
grid, which I think it's not correct.

> (The flexbox testcase is actually exactly the same, it's just that
> the 'background-color:lightgrey' was omitted for the flexbox containers
> (by mistake I assume).  Adding it makes the grid/flexbox tests look
> the same.)

Well, actually, FF also uses the flexbox container's content-box as
baseline for the second flexbox, at least in the build I've got now.
So I'd say the flexbox cases are relevant and we should clarify the
different behavior.

> 
> Javier, do you agree with the above analysis and that this is
> the correct rendering?

I mostly agree with the general approach, but there are still
some cases I still don't fully understand.

Comment 7

[Mats Palmgren (inactive)]

(In reply to Javier Fernandez from comment #6)
> However, currently Flexbox uses the container's content-box as synthesized
> baseline when in an inline-level context. The second grid is indeed an
> inline-block and it's part of the inline-block used for rendering the text
> line.

This is wrong.  The spec is CSS Alignment spec is clear that the margin-box
should be used for synthesizing baselines in an inline-level context.

https://drafts.csswg.org/css-align-3/#baseline-export
"
Note: The edges used to synthesize baselines from a box depend on their formatting context: inline-level boxes synthesize from their margin edges [CSS-INLINE-3], table cells synthesize from their content edges [CSS2], and grid and flex items synthesize from their border edges [CSS-GRID-1] [CSS-FLEXBOX-1].
"

> I decided, for now, to emulate this behavior in grid, but sincerely, I
> couldn't find out where in the spec it's stated such behavior.

I intend to implement what the spec says above.  I strongly suggest you
do the same.  I've filed:
https://bugs.chromium.org/p/chromium/issues/detail?id=671132

> Well, actually, FF also uses the flexbox container's content-box as
> baseline for the second flexbox, at least in the build I've got now.
> So I'd say the flexbox cases are relevant and we should clarify the
> different behavior.

When synthesizing a baseline in an inline-level context, the margin edges
should be used for all boxes, including Flexbox containers.
I intend to fix our Flexbox/Grid implementations to follow the spec.

Comment 8

[Javier Fernandez]

(In reply to Mats Palmgren (:mats) from comment #7)
> (In reply to Javier Fernandez from comment #6)
> > However, currently Flexbox uses the container's content-box as synthesized
> > baseline when in an inline-level context. The second grid is indeed an
> > inline-block and it's part of the inline-block used for rendering the text
> > line.
> 
> This is wrong.  The spec is CSS Alignment spec is clear that the margin-box
> should be used for synthesizing baselines in an inline-level context.
> 
> https://drafts.csswg.org/css-align-3/#baseline-export
> "
> Note: The edges used to synthesize baselines from a box depend on their
> formatting context: inline-level boxes synthesize from their margin edges
> [CSS-INLINE-3], table cells synthesize from their content edges [CSS2], and
> grid and flex items synthesize from their border edges [CSS-GRID-1]
> [CSS-FLEXBOX-1].
> "
> 

I agree, the spec is pretty clear, indeed. 

> > I decided, for now, to emulate this behavior in grid, but sincerely, I
> > couldn't find out where in the spec it's stated such behavior.
> 
> I intend to implement what the spec says above.  I strongly suggest you
> do the same.  I've filed:
> https://bugs.chromium.org/p/chromium/issues/detail?id=671132
> 

Thanks for reporting, I'll do the same and implement this according
to the specs.

> > Well, actually, FF also uses the flexbox container's content-box as
> > baseline for the second flexbox, at least in the build I've got now.
> > So I'd say the flexbox cases are relevant and we should clarify the
> > different behavior.
> 
> When synthesizing a baseline in an inline-level context, the margin edges
> should be used for all boxes, including Flexbox containers.
> I intend to fix our Flexbox/Grid implementations to follow the spec.

Great thanks again for the discussion.

Comment 9

[Mats Palmgren (inactive)]

Attachment: [css-grid] Synthesize a grid container baseline from the margin-box when in an inline-level context, and from the border-box otherwise.

(In reply to Daniel Holbert [:dholbert] from comment #3)
> > +           aFrame->HasAnyStateBits(NS_STATE_GRID_SYNTHESIZE_BASELINE)) ||
> > +          (fType == nsGkAtoms::flexContainerFrame &&
> > +           aFrame->HasAnyStateBits(NS_STATE_FLEX_SYNTHESIZE_BASELINE)) ||
> > +          (fType == nsGkAtoms::tableWrapperFrame &&
> > +           static_cast<const nsTableWrapperFrame*>(aFrame)->GetRowCount() == 0)) {
> 
> (Is GetRowCount() really all we need to check here, for tables?  What if we
> have an empty table which has a caption -- should we be falling back on that
> for the baseline? If we're not sure, perhaps it's better to punt on tables
> for now?)

Yes, I believe it's correct.
https://drafts.csswg.org/css-align-3/#baseline-export
The spec is a bit vague about synthesizing baselines when various table boxes
are missing, and UAs are incompatible in a lot of cases here.  I intend to
dig deeper (later) and then file spec bugs to have that sorted out.
But for now, I think the above is good enough.
(Note: the word "caption" is non-existent in the CSS Align spec.)

https://drafts.csswg.org/css-tables-3/#fixup-algorithm
mentions briefly that "The table-root box (not the table-wrapper box) is used
when doing baseline vertical alignment for an inline-table. " so since
the caption is outside the table-root box it can't possibly be involved.
(Disclaimer: I haven't really tested what UAs do when there's a caption
but no row-group/row/cell)

> Is there a reason you're putting this on nsFrame instead of nsIFrame?

I think the principle is that methods that are only needed internally
by frame classes themselves to implement reflow etc should go in nsFrame
(or subclasses).  Methods that are needed by external (to layout) should
go in nsIFrame.  Roughly.

I've moved it for now since I needed it from nsIFrame methods in
later patches, but I may move it again in the future... :-)
(synthesizing baselines is something that I consider "layout-internal")

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8816877 [details] [diff] [review]
[css-grid] Synthesize a grid container baseline from the margin-box when in an inline-level context, and from the border-box otherwise.

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

r=me

::: layout/generic/nsFrameStateBits.h
@@ +312,5 @@
>  FRAME_STATE_BIT(FlexContainer, 21, NS_STATE_FLEX_IS_LEGACY_WEBKIT_BOX)
>  
> +// True if the container has no flex items; may lie if there is a pending reflow
> +// XXX not used yet...
> +FRAME_STATE_BIT(FlexContainer, 22, NS_STATE_FLEX_SYNTHESIZE_BASELINE)

Note: this XXX comment should probably be removed as part of bug 1313811's patch. (Doesn't look like that happens in the current version of the patch over there, but it should.)

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Note: this XXX comment should probably be removed as part of bug 1313811's
> patch. (Doesn't look like that happens in the current version of the patch
> over there, but it should.)

Oops, sorry, ignore this.  This was just my workflow failing me when I reviewed the other bug. (I imported that other bug's patch in isolation, applied it on its own, & viewed the result in a merge tool. The xxx-comment-removal didn't show up for me, simply because I hadn't imported the patch that added it. Mercurial had complained about that when I applied the patch, but I didn't notice at first. :)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d97a9484c12
[css-grid] Synthesize a grid container baseline from the margin-box when in an inline-level context, and from the border-box otherwise.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/df9b7aff67b2
[css-grid] Reftests for synthesize baselines for empty grid containers.

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5d97a9484c12
https://hg.mozilla.org/mozilla-central/rev/df9b7aff67b2

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 8816877 [details] [diff] [review]
[css-grid] Synthesize a grid container baseline from the margin-box when in an inline-level context, and from the border-box otherwise.

css-grid fix for aurora52

Comment 15

[Carsten Book [:Tomcat]]

has problems to apply to aurora

grafting 380512:5d97a9484c12 "Bug 1313068 - [css-grid] Synthesize a grid container baseline from the margin-box when in an inline-level context, and from the border-box otherwise.  r=dholbert"
merging layout/base/nsLayoutUtils.cpp
merging layout/generic/nsFrameStateBits.h
merging layout/generic/nsGridContainerFrame.cpp
merging layout/generic/nsIFrame.h
warning: conflicts while merging layout/generic/nsFrameStateBits.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 16

[Mats Palmgren (inactive)]

Landed in Aurora revs: b1a830373847, 9bf667428df5

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

we added:
== grid-container-synthesized-baseline-001-ref.html grid-container-synthesized-baseline-001-ref.html

but this file was added as well and never referenced:
https://searchfox.org/mozilla-central/source/layout/reftests/css-grid/grid-container-synthesized-baseline-001.html

:mats, should we be using the -001.html file, or should we delete it?

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ace527e70c25
(follow-up) - Fix typo in reftest manifest.  r=me

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ace527e70c25