1324321 - Sort out and document the aWritingMode param on GetLogicalBaseline() et al

Status: NEW

(Core :: Layout, enhancement, P3)

Description

[Mats Palmgren (inactive)]

There's some confusion about which frame's WM should be used in different
places in our code when calculating baselines (natural or synthesized).

Here's a few example calls to GetLogicalBaseline:
http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/layout/generic/nsFlexContainerFrame.cpp#458
where 'mWM' is that of 'mFrame' (the flex item), whereas
http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/layout/forms/nsHTMLButtonControlFrame.cpp#316
'wm' is that of 'aFirstKid->GetParent()'.
http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/layout/generic/nsTextFrame.cpp#10104
suggests it could be either 'this' or its parent?

This is the one I found most interesting:
http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/layout/generic/nsLineLayout.cpp#1431,1436,1440
First, note that we explicitly avoid calling GetLogicalBaseline
for IsOrthogonal() WMs (the test on line 1436 is actually stricter
than that).  This, and some calls in other places, suggests that
GetLogicalBaseline should never be called with an orthogonal WM.

OK, so where does 'lineWM' come from? mRootSpan->mWritingMode is set here:
http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/layout/generic/nsLineLayout.cpp#213
which comes from:
http://searchfox.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp#3864,3888
which is the block's WM, with RTL/LTR direction set based on the
first frame on the line (IIUC):
http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/layout/generic/nsIFrame.h#748-754
http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/layout/generic/nsFrame.cpp#1120

Looking at GetLogicalBaseline implementations, there are three typical
uses of aWM:
1. "aWritingMode.IsLineInverted()" for "flipping the coordinates":
http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/layout/generic/nsFrame.cpp#1470
2. "aWM.IsAlphabeticalBaseline()" for choosing an alphabetical/central
baseline (i.e. the 'auto' behavior from the spec above)
http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/layout/generic/nsGridContainerFrame.cpp#179
(here, the WM comes from the grid container for sure though)
http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/layout/generic/nsLineLayout.cpp#2060
same thing in line layout.
3. BSize(aWM), margin.BStart(aWM), margin.BEnd(aWM) etc for
calculating the baseline position.

So, which aWM do these three uses require to give the correct result
per the specs?  First, let's see what the specs say:

The CSS Box Alignment spec states:
https://drafts.csswg.org/css-align-3/#align-by-baseline
"... the alignment baseline is the dominant baseline of the alignment context."

The CSS Inline Layout spec states:
https://drafts.csswg.org/css-inline/#alignment-baseline-property
"'baseline'
     Use the dominant baseline choice of the parent. Match the box’s
     corresponding baseline to that of its parent. "

https://drafts.csswg.org/css-inline/#propdef-dominant-baseline
"'auto'
    Equivalent to alphabetic in horizontal writing modes and in vertical
    writing modes when text-orientation is sideways, sideways-right, or
    sideways-left. Equivalent to central in vertical writing modes when
    text-orientation is mixed or upright. "

(we don't support either of these properties directly, but the above
 is what applies for 'vertical-align:baseline' AFAICT.)

https://drafts.csswg.org/css-writing-modes-3/#baseline-alignment
"E.g. if the parent’s dominant baseline is alphabetic, then the child’s
alphabetic baseline is matched to the parent’s alphabetic baseline,
even if the child’s dominant baseline is something else."

It seems to me that the specs say that the parent's WM decides
which baseline to use, so that means 2=parent's.
If we require that all calls to GetLogicalBaseline are on frames
that are parallel to its parent, then 3 will work with either.
(if they are orthogonal, I suspect 3. is nonsensical in most cases)
I don't know much about 1. -- it seems to be used for choosing
the correct side(*) for alphabetical baselines only, so maybe
that means 1=parent too?
(*) i.e. the 'line-orientation:under' side?
https://drafts.csswg.org/css-writing-modes-3/#line-directions

So, I think for GetLogicalBaseline, documenting aWM as "the alignment
context's WM" is roughly correct, with RTL/LTR tweak noted above.
For Grid/Flexbox, I think we might be using the wrong WM in some cases,
I haven't reviewed those in detail yet.

For a future baseline API, that supports the 'dominant/alignment-
baseline' properties, perhaps we need both WMs?  How should we signal
which kind of baseline (alphabetic/central/mathematical etc) we want?
Is there anything else we want to take into account now to support
baseline alignment fully per all the specs above?

(BTW, I'm landing a half-finished new baseline API in bug 1312379
to improve support for exporting baselines from grid/flexbox
containers, to be uplifted to Aurora for the Grid release.
I'll continue to work on it and finish that API in v53 I hope.)

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase #1

BTW, I submitted this to Try to investigate the 'lineWM' vs 'pfd->mFrame->GetWritingMode()'
https://hg.mozilla.org/try/rev/8aad7ddfd5c58000fc2226068af4dcf93e13b0c5
and it passed reftest!

In retrospect, I'm pretty sure IsLineInverted() can't differ
there, since we know BlockDir() is the same here.  But, IsSideways()
can certainly differ!  Here's a simplified test that triggers that
assertion.

Comment 2

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Here's a more comprehensive test where that first test was reduced from.

Jonathan, can you check that our rendering of this is correct?
The rendering in Chrome is quite different and I figured I should
report it to them.

Comment 3

[Mats Palmgren (inactive)]

(BTW, note the 'border-block-start-color: lime' for orientation :-) )

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: testcase to compare firefox vs chrome baseline handling

I'm pretty sure Chrome's current behavior is incorrect (per spec) in at least some cases, and yields results that are inferior to Firefox.

Consider this example. The blue-bordered inline box is simple: it shares the same centered baseline alignment as the outer div (which provides the parent line). The red-bordered inline box, however, has text-orientation:sideways and therefore uses an alphabet baseline internally for its contents.

But in terms of the inline's alignment within the overall line, its *center* baseline should align with the parent's center baseline, just like the blue box. What Chrome seems to be doing is aligning the child's *alphabetic* baseline with the parent's *center*, which does not match the CSS Inline Layout spec (as quoted above).

I haven't tried to check through all the combinations in attachment 8819706 [details] yet, but at least for attachment 8819705 [details] I believe we're doing the right thing, and Chrome is wrong (as per this example, which perhaps makes it clearer _why_ they're wrong).

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Please do file a Chromium bug with the problems you find if there isn't one already.

Comment 6

[Jonathan Kew [:jfkthame]]

See https://bugs.chromium.org/p/chromium/issues/detail?id=677801.

Comment 7

[Jonathan Kew [:jfkthame]]

Note that although on testcase #1 (attachment 8819705 [details]), I think our behavior is right (and Chromium is wrong), the collection of examples in testcase #2 (attachment 8819706 [details]) includes plenty where our behavior looks wrong as well.

For example, it seems that we don't handle mixtures of vertical-lr and vertical-rl very well when text-orientation is sideways:

data:text/html,<div style="text-orientation:sideways;writing-mode:vertical-lr">abc<span style="writing-mode:vertical-rl">def

data:text/html,<div style="text-orientation:sideways;writing-mode:vertical-rl">abc<span style="writing-mode:vertical-lr">def

(This seems like an obscure case that's unlikely to be used in practice, but nevertheless, it suggests we aren't always handling baselines correctly.)

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.