Closed Bug 1739561 Opened 1 year ago Closed 8 months ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
103 Branch
Webcompat Priority P2
Tracking Status
firefox-esr91 --- wontfix
firefox94 --- wontfix
firefox95 --- wontfix
firefox96 --- wontfix
firefox97 --- wontfix
firefox101 --- wontfix
firefox102 --- wontfix
firefox103 --- verified

People

(Reporter: dholbert, Assigned: TYLin)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(6 files)

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.)

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

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?

Flags: needinfo?(aethanyc)
Regressions: 1622935

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.

Regressed by: 1622935
No longer regressions: 1622935
Has Regression Range: --- → yes

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).

Component: Layout: Flexbox → Layout: Columns
Depends on: 793686
Flags: needinfo?(aethanyc)

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.

Component: Layout: Columns → Layout: Flexbox

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

Blocks: 1695475
Blocks: 939897
No longer blocks: 1695475

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)) {

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

Flags: needinfo?(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?

Flags: needinfo?(emilio) → needinfo?(aethanyc)

(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'
Flags: needinfo?(aethanyc)
Priority: -- → P2

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.

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

Flags: needinfo?(fgriffith)

I'll take a look.

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

Assignee: nobody → aethanyc
Flags: needinfo?(fgriffith)

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

See Also: → park-wta

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

Webcompat Priority: --- → ?
Flags: needinfo?(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.

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.

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.

Status: NEW → ASSIGNED
Flags: needinfo?(dholbert)
Webcompat Priority: ? → P2

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).

Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/466d30a90a01
Honor break-inside:avoid on flex containers. r=dholbert
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch

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.

Flags: needinfo?(aethanyc)

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

Flags: needinfo?(aethanyc)

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!

Status: RESOLVED → VERIFIED

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

Regressions: 1776079
You need to log in before you can comment on or make changes to this bug.