969874 - Incorrect baseline alignment for flex container with overflow other than visible

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Roman Komarov]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.12786 YaBrowser/13.12.1599.12786 Safari/537.36

Steps to reproduce:

1. Add some blocks with `display: inline-flex`
2. Give some of them `overflow` other than `visible`.
3. Use `vertical-align: baseline` for them.

See this example: http://dabblet.com/gist/8886089, note that in all other browsers it behave properly.



Actual results:

Element with `display: inline-flex` and `overflow` other than `visible` have an incorrect baseline.


Expected results:

The baseline for block with `overflow` other than `visible` should be at the first line box's baseline.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: reporter's testcase

Comment 2

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

CSS 2.1 is reasonably clear that the concept of last-line baseline does not cross overflow != visible boundaries, with the text:

The baseline of an 'inline-block' is the baseline of its last line box in the normal flow, unless it has either no in-flow line boxes or if its 'overflow' property has a computed value other than 'visible', in which case the baseline is the bottom margin edge.
( http://www.w3.org/TR/CSS21/visudet.html#propdef-vertical-align )

but the concept of first-line baseline does cross overflow != visible boundaries:

The baseline of a cell is the baseline of the first in-flow line box in the cell, or the first in-flow table-row in the cell, whichever comes first. If there is no such line box or table-row, the baseline is the bottom of content edge of the cell box. For the purposes of finding a baseline, in-flow boxes with a scrolling mechanisms (see the 'overflow' property) must be considered as if scrolled to their origin position. Note that the baseline of a cell may end up below its bottom border, see the example below. 
( http://www.w3.org/TR/CSS21/tables.html#height-layout )

Do we have the same bug for table cells?  (Is it filed?)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase with 'inline-table', containing an overflowing <div>

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #2)
> CSS 2.1 is reasonably clear that the concept of last-line baseline does not
> cross overflow != visible boundaries
[...]
> but the concept of first-line baseline does cross overflow != visible
> boundaries:

Interesting distinction.  So that implies that this is a valid bug, since we use a flex item's first line as the container's baseline.

> Do we have the same bug for table cells?

No, we don't seem to have the same bug with table-cells.  Here's a testcase demonstrating that, with an "overflow:scroll" on a <div> inside of an inline-table. It successfully baseline-aligns (using its first line) with text outside the table.

(side note: I initially tried this with 'overflow' directly set on a table-cell (both <td> and <div style="display:table-cell"), but that didn't work, because apparently 'overflow' has no rendering effect on table-cells. So this testcase has 'overflow' set on a <div>, which gets wrapped in a table-cell.)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 1

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: reference case 1

Comment 6

[Lev Solntsev]

overflow:hidden may have effect on table cells if there fixed table layout, i.e. table-layout:fixed is set on table.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Lev Solntsev from comment #6)
> overflow:hidden may have effect on table cells if there fixed table layout,
> i.e. table-layout:fixed is set on table.

(That doesn't seem to make overflow:hidden or overflow:scroll do anything interesting on my end, from a few attempts with my attached "testcase with 'inline-table'", attachment 8373641 [details])

Comment 8

[Roman Komarov]

I guess, the Flexbox spec (http://www.w3.org/TR/css3-flexbox/#flex-baselines) should be mentioned here as well.

Regarding the subject of a bug:

> if the box contributing a baseline has an ‘overflow’ value that allows scrolling, the box must be treated as being in its initial scroll position for the purpose of determining its baseline.

Comment 10

[Ilya]

Surprisingly, it appears that the experimental implementation of Flexbox 2009 (display: -moz-inline-box) doesn't have this bug: http://dabblet.com/gist/3b91b713f8a6db0a50d2

What is the difference between these implementations relating to baseline? Couldn't the old baseline positioning be just ported into new implementation?

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Ilya from comment #10)
> Surprisingly, it appears that the experimental implementation of Flexbox
> 2009 (display: -moz-inline-box) doesn't have this bug: [...]
> 
> What is the difference between these implementations relating to baseline?
> Couldn't the old baseline positioning be just ported into new implementation?

(Sorry for the delayed response)

-moz-inline-box is a XUL thing -- we're better off porting behavior from display:inline-block rather than from XUL box layout.

Also, for the record: https://github.com/webcompat/web-bugs/issues/17781 is effectively an instance of this bug affecting twitter.com.

Comment 13

[Daniel Holbert [:dholbert]]

As discussed on dupe-bug 1471942, this now has clearer spec text (which indicates that inline-block is special for legacy reasons & other scrollable inline boxes should Just Work).  Link:  https://drafts.csswg.org/css-align/#baseline-export

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

This appears to be causing layout issues on the new twitter design.

Comment 17

[Daniel Holbert [:dholbert]]

I'm taking a look at this.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: Bug 969874: Make scrollable frames derive their baseline from their scrolled content (unless their display value is block-inside). r?mats

Before this patch, we made scrollable frames derive their baseline from their
margin-box, because that's what the spec requires for scrollable inline-block
boxes. However, the spec now singles out inline-block as a special case, and
other sorts of scrollable inline-level containers are supposed to derive their
baseline from the scrolled content. So, this patch makes us do that, with an
exception for scrollable inline-block boxes.

For more info about the inline-block special case, see the end of the "block
containers" chunk here: https://drafts.csswg.org/css-align/#baseline-export

Comment 19

[Daniel Holbert [:dholbert]]

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

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: screenshot of Twitter on Nightly, vs. patched Nightly, vs. Chrome

Here's a screenshot of the Twitter UI that was broken due to this bug (look at the "N Retweets/Likes" baseline alignment at the bottom).

Leftmost is Firefox Nightly (broken), middle is my build with the patch applied, and right is Chrome for reference.

As you can see, the baseline alignment seems to be good (and identical) between patched-Firefox and Chrome.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21b74260c8a9
Make scrollable frames derive their baseline from their scrolled content (unless their display value is block-inside). r=mats

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Daniel, does this patch really want to use IsBlockFrame() rather than, e.g., IsFrameOfType(eBlock)? IsBlockFrame only returns true for actual nsBlockFrames, not for subclasses like ComboboxDisplayFrame or DetailsFrame.

Comment 23

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/21b74260c8a9

Comment 24

[Daniel Holbert [:dholbert]]

Ah, you're right emilio - the patch broke e.g. this testcase (to now align with the [hidden] last-line baseline of the text inside the details frame): https://jsfiddle.net/xqf1w097/

I should indeed use IsFrameOfType(eBlock) to avoid getting the New Behavior for block subclasses. I'll file a followup on that.

Comment 25

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

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

Comment 26

[Daniel Holbert [:dholbert]]

[clearing ni, as it's addressed by bug 1525628]

Comment 27

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

Upstream PR merged