1525133 - nsBlockFrame::GetNaturalBaselineBOffset calls kid->GetVerticalAlignBaseline() without checking whether its WM is orthogonal

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, enhancement, P3)

Description

[Daniel Holbert [:dholbert]]

nsBlockFrame::GetNaturalBaselineBOffset has a call to...

kid->GetVerticalAlignBaseline(aWM)

...without checking whether aWM is parallel to kid's writing mode.

This is bogus, per the GetVerticalAlignBaseline documentation:

• @note You should only call this on frames with a WM that's parallel to aWM.
  https://searchfox.org/mozilla-central/rev/152993fa346c8fd9296e4cd6622234a664f53341/layout/generic/nsIFrame.h#1406

We should add a writing mode check before the call in question.

(I ran into this when fixing bug 969874, when trying to add a more subtle implementation of GetLogicalBaseline. Inside that implementation, I assumed that the passed-in aWM was parallel to the this frame's WM, and that assumption turned out not to be valid, in part due to this bug.)

Comment 1

[Daniel Holbert [:dholbert]]

The problematic call is here:
https://searchfox.org/mozilla-central/rev/152993fa346c8fd9296e4cd6622234a664f53341/layout/generic/nsBlockFrame.cpp#490
...which comes from bug 1312379. Adding dependency on that bug.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 3

[Daniel Holbert [:dholbert]]

Chrome 73 and Edge 18 render "outside --> <-- inside" lined up in the attached testcase, and doesn't change the alignment when you hover the vertical text (increasing its font size).

Firefox Nightly renders outside--> higher up than "<--inside" (at a kind of arbitrary point that depends on the size of the vertical text), and it changes the position of "outside-->" when you hover the vertical text to adjust its font size.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (with overflow:hidden vertical WM last child inside of inline-block)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Bug 1525133: Explicitly skip orthogonal-flow children when determining last-baseline of a block from its children. r?mats

Without the check that I'm adding in this patch, we'd violate the
"parallel writing mode" expectation of some baseline accessors
that we use in the now-guarded code. And we'd produce bogus layout
as a result.

The added assertions are just for good measure. The included testcase
causes us to fail both assertions, in a build that's missing the fix.

Comment 6

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82b600024b1959d175545705867ce520f3029891

(including bug 969874 which layers on top of this)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbdbcc95849b
Explicitly skip orthogonal-flow children when determining last-baseline of a block from its children. r=mats

Comment 8

[Oana Pop-Rus]

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

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15274 for changes under testing/web-platform/tests

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged