1306894 - [css-flexbox] nsFlexContainerFrame needs to implement GetLogicalBaseline

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Mats Palmgren (inactive)]

Attachment: testcase

Comment 1

[Mats Palmgren (inactive)]

BTW, I think bug 1307806 is also needed to make the testcase work as expected.

Comment 2

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert])
> (Yeah, the flex container's baseline gets set (not-entirely-correctly) here:
> >   aDesiredSize.SetBlockStartAscent(flexContainerAscent);
> https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp#4217
> 
> We could cache that on the frame in a member-variable, if that tends to
> match what we do elsewhere for storing baselines.)

Sounds good to me, with the usual NS_INTRINSIC_WIDTH_UNKNOWN init + assertion.

Comment 3

[Brad Werth [:bradwerth]]

Attachment: Bug 1306894 Part 1: Cache baseline from nsFlexContainerFrame::Reflow() for use in later calls to GetLogicalBaseline().

Review commit: https://reviewboard.mozilla.org/r/84982/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84982/

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8799910 [details]
Bug 1306894 Part 1: Cache baseline from nsFlexContainerFrame::Reflow() for use in later calls to GetLogicalBaseline().

https://reviewboard.mozilla.org/r/84982/#review83552

r=me with "virtual" nit addressed

::: layout/generic/nsFlexContainerFrame.h:80
(Diff revision 1)
>    virtual nsIAtom* GetType() const override;
>  #ifdef DEBUG_FRAME_DUMP
>    virtual nsresult GetFrameName(nsAString& aResult) const override;
>  #endif
> +
> +  virtual nscoord GetLogicalBaseline(mozilla::WritingMode aWM) const override;

Don't use the "virtual" keyword here. ("override" already implies it, which is why our coding style guide says to only use one of override|virtual|final.)

The contextual code does use both, but that's simply because it's from before we added that coding style guideline (and we have yet to clean it up).

::: layout/generic/nsFlexContainerFrame.cpp:2141
(Diff revision 1)
> +nsFlexContainerFrame::GetLogicalBaseline(mozilla::WritingMode aWM) const
> +{
> +  NS_ASSERTION(mBaselineFromLastReflow != NS_INTRINSIC_WIDTH_UNKNOWN,
> +               "baseline has not been set");
> +
> +  return mBaselineFromLastReflow;

I wonder if we should be doing something with |aWM| here? (probably comparing it to our own)

I guess we don't do that in other implementations, like in nsGridContainerFrame.h, nsBRFrame.cpp, & to a lesser extent nsFormControlFrame.cpp. Maybe we're getting lucky about the way this function happens to be called...

Anyway, this is probably fine for now.

Comment 5

[Brad Werth [:bradwerth]]

Comment on attachment 8799910 [details]
Bug 1306894 Part 1: Cache baseline from nsFlexContainerFrame::Reflow() for use in later calls to GetLogicalBaseline().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84982/diff/1-2/

Comment 6

[Mats Palmgren (inactive)]

Lovely!  Does the attached testcase work with this change?

If so, could you please uncomment the TODO case in this reftest?
http://searchfox.org/mozilla-central/source/layout/reftests/css-grid/grid-container-baselines-004.html
(hopefully it should just work)

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1306894 Part 2: Update the grid-container-baselines-004 reftest to exercise baseline alignment of flexboxes within grid containers.

Review commit: https://reviewboard.mozilla.org/r/85016/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85016/

Comment 8

[Brad Werth [:bradwerth]]

(In reply to Mats Palmgren (:mats) from comment #6)
> Lovely!  Does the attached testcase work with this change?

Yes, it does fix the attached testcase. Thank you for pointing out grid-container-baselines-004 -- I was thinking I'd have to create a new reftest. Part 2 of the patch updates that reftest.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8799965 [details]
Bug 1306894 Part 2: Update the grid-container-baselines-004 reftest to exercise baseline alignment of flexboxes within grid containers.

https://reviewboard.mozilla.org/r/85016/#review83568

Two commit message nits:
> Updated the grid-container-baselines-004 reftest.

(1) s/Updated/Update/ (we tend to use *present* active tense, rather than past tense, when describing the change in commit message)

(2) It'd be nice to have a bit more detail on the change here. "Update this test" doesn't really say much. Better would be: "Uncomment the flex-container-dependent part of grid-container-baselines-004 reftest."


(I had these same thoughts about parts 3-9 on bug 1221565, but I didn't bring them up there because it would've been a lot of commit-message-rewriting, for marginal benefit. :) I'm bringing them up here since now there's only one patch to update, and since I think they're good best practices to get used to.)

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8799965 [details]
Bug 1306894 Part 2: Update the grid-container-baselines-004 reftest to exercise baseline alignment of flexboxes within grid containers.

https://reviewboard.mozilla.org/r/85016/#review83572

[sorry, I meant to set this to "r+" with my previous comment -- r=me with those commit message nits addressed.]

Comment 11

[Brad Werth [:bradwerth]]

Comment on attachment 8799965 [details]
Bug 1306894 Part 2: Update the grid-container-baselines-004 reftest to exercise baseline alignment of flexboxes within grid containers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85016/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e88bcc7f1bc
Part 1: Cache baseline from nsFlexContainerFrame::Reflow() for use in later calls to GetLogicalBaseline(). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/570c8ad43113
Part 2: Update the grid-container-baselines-004 reftest to exercise baseline alignment of flexboxes within grid containers. r=dholbert

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e88bcc7f1bc
https://hg.mozilla.org/mozilla-central/rev/570c8ad43113