1739561 - Broken "Trips" flight preview in Gmail, with flexbox nested in multicol

Status: VERIFIED FIXED

(Core :: Layout: Flexbox, defect, P2)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (somewhat minimized)

STR:

1. Load attatched testcase

(which I captured from actual Gmail -- this shows up as a card at the top of an email for a United Airlines "eTicket Itinerary and Receipt" email for a flight that I just booked. I used Testcase Reducer and then edited the data to change/remove personal details)

ACTUAL RESULTS:
Flight duration is at the bottom of the first column, and then 12 hr, 0 min is at the top of the second column. The yellow box alongside them is sliced in half, too.

EXPECTED RESULTS:
The header Flight duration and its value 12 hr, 0 min should be in the same column. (And the yellow box alongside them should be intact.)

In actual Gmail, there's an icon instead of the yellow box (and the icon gets sliced). The specific icon is https://www.gstatic.com/images/icons/material/system_gm/1x/schedule_black_20dp.png and it's drawn as a background-image on the element in question (and the element gets sliced up, so the background-image does too.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screenshot of testcase (Firefox on top, Chrome on bottom)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: screenshot showing how this looks in actual Gmail (redacted w/ solid red blocks, brokenness highlighted with empty red rectangles)

Comment 3

[Daniel Holbert [:dholbert]]

mozregression says this regressed in bug 1622935, which is not too surprising given the content (flexbox-in-multicol).

In builds before that, we basically refused to fragment the flex containers here (because we couldn't), which happened to mean we matched Chrome. Now we do fragment the flex container, which happens to produce this unwanted split-up layout. Chrome doesn't fragment any of the flex containers for some reason (though I have seen progress on https://bugs.chromium.org/p/chromium/issues/detail?id=660611 and I wonder if maybe they might start fragmenting more eagerly once that is fixed).

TYLin, do you have cycles to take a look here? Is there any particular justification for Chrome's layout vs. our layout here, and anything we should suggest to the Gmail team to help us avoid this pitfall, and/or anything we should fix on our end?

Comment 4

[Ian Kilpatrick [:iank]]

We (Blink) do fragment flex containers (which works some of the time), but here we are respecting "break-inside: avoid;" on the ".tk" class in the example. If you remove that rule (and recreate the multicol - e.g. toggling the columns property) we have a similar (broken) behaviour.

Comment 5

[Daniel Holbert [:dholbert]]

Aha, thanks! I did have a thought that break-inside:avoid might be a useful thing here, but didn't bother to look for it in the testcase.

So this is a version of bug 793686, then, basically (with the wrinkle that the child being broken here is something that we only ~recently started fragmenting, which is why this particular use case "regressed" more recently than when bug 793686 was filed).

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 2: similar breakage without flexbox (just block in multicol with break-inside:avoid)

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

In our current fragmentation architecture, each container are required to handle break-inside property. We currently do not handle break-inside property in flex container at all.

We should do something similar like this part in nsFieldSetFrame::Reflow by using ShouldAvoidBreakInside() and return SetInlineLineBreakBeforeAndReset() status to the parent.

Comment 8

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

Set release status flags based on info from the regressing bug 1622935

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment 6 is because the columns code hits the mAtTopOfPage code-path, and this might be a reasonable fix for it:

diff --git a/layout/generic/nsContainerFrame.cpp b/layout/generic/nsContainerFrame.cpp
index 1ef135d225ff7..e47e317d3fba8 100644
--- a/layout/generic/nsContainerFrame.cpp
+++ b/layout/generic/nsContainerFrame.cpp
@@ -2736,7 +2736,8 @@ bool nsContainerFrame::ShouldAvoidBreakInside(
   if (!mayAvoidBreak) {
     return false;
   }
-  if (aReflowInput.mFlags.mIsTopOfPage) {
+  if (aReflowInput.mFlags.mIsTopOfPage &&
+      aReflowInput.mBreakType == ReflowInput::BreakType::Page) {
     return false;
   }
   if (IsAbsolutelyPositioned(disp)) {

Comment 10

[Emilio Cobos Álvarez (:emilio)]

ni? me to potentially land comment 9 in a separate bug.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Yeah, so try is not super-impressed with comment 9. It does cause a bunch of new tests to pass, but it causes others to time out (presumably with some sort of infinite column-balancing reflow): https://treeherder.mozilla.org/jobs?repo=try&revision=6b7b3f769692e6188450426a075298fe9f651db1

Ting-Yu, do you have any strong opinions on ^? Should I try to dig into those timeouts?

Comment 12

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(presumably with some sort of infinite column-balancing reflow)

Comment 9 is trying to avoid breaking when break-inside: avoid and break-inside: avoid-column. I guess the unbreakable child is stuck in the column-balancing loop because the column keep reporting unfit.

I feel the solution would be: the container frame (block, flex, etc) should properly place its unbreakable child regardless its child is at the top of page/column or not. Ideally, we don't need

if (aReflowInput.mFlags.mIsTopOfPage) {
   return false;
}

to force the container to place the unbreakable top-of-page child at the top of page/column.

Ting-Yu, do you have any strong opinions on ^? Should I try to dig into those timeouts?

Yeah, it would be great if you have extra cycles to dig if the reason of the timeout, but no pressure.

Tip: It might be helpful to enable column logs in ~/.mozbuild/machrc by

[runprefs]
logging.ColumnSet = 'debug'

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Visible oddness on a high-profile site like GMail is concerning. Not going to make 96 at this point but it would be good if we could try to get this sorted for 97.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Frank, can we try to bump the priority here? It's an old issue, but a pretty major site to have visible bustage on.

Comment 15

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

I'll take a look.

Emilio, if you have any follow-up investigation per comment 11, I'd love to hear your result :)

Comment 16

[Frank Griffith Jr]

Ryan....discussed with both Ting Yu and Daniel on 1/25. Ting Yu will be working on this.

Comment 17

[Marco Castelluccio [:marco]]

Given that Ting Yu is off until May 8, could we find somebody else to work on this?

Comment 18

[Daniel Holbert [:dholbert]]

Yeah, I'm tentatively planning on working on this while TYLin is away (though it's queued up behind a few other tasks at the moment).

I'll leave needinfo open.

Comment 19

[Frank Griffith Jr]

Daniel, let's put this on the triage agenda on Monday to see if anyone else might be in a position to pitch in as well.

Comment 20

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

I'm back from my leave this week and working on this bug. I'm experimenting the idea in comment 7. Hoping I can have a patch soon.

Comment 21

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1739561 - Honor break-inside:avoid on flex containers.

Note this patch solves only the cases where there are more than one flex
containers with (break-inside:avoid) under multicol. When there is only one
element (either flex container or block container), multicol balancing can still
break it (bug 793686).

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/466d30a90a01
Honor break-inside:avoid on flex containers. r=dholbert

Comment 23

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/466d30a90a01

Comment 24

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

The patch landed in nightly and beta is affected.
:TYLin, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox102 to wontfix.

For more information, please visit auto_nag documentation.

Comment 25

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

I'd like to have this patch ride the train since fragmentation issues are always complex even if the patch looks small.

Comment 26

[Daniel Holbert [:dholbert]]

Attachment: screenshot showing this fixed in actual-Gmail

I retested this in actual Gmail and confirmed that it's fixed in Nightly (and also confirmed that I can still reproduce the bug in Firefox release version 101.0, which renders the Gmail content as-shown in attachment 9249374 [details]).

The content has very-slightly changed (e.g. now the "View in Trips" has been replaced with an auto-collapsed click-to-expand view of my return flight, just off the bottom of this screenshot); but otherwise it's the same as when I filed this, and it looks great in Nightly.

Thanks, TYLin!

Comment 27

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Thanks for verifying this bug on the actual Gmail content, dholbert!