Issues in Layout that do not fit into any other Layout component or which span multiple Layout components.

Bugs related to the top level presentation objects (pres shell, pres context, and document viewer), the frame constructor, and the base frame classes, as well as general issues with alignment and sizing, all belong here.

As part of removing/deprecating XUL, it'd be useful to test how our UI works if we swap in modern flexbox as the implementation for -moz-box (kind of like we did for -webkit-box in bug 1208635).
Filing this bug on that. Probably nothing will land here -- this will just be a place for coming up with a good prototype patch, from which we can perhaps iterate on finding bits of UI that need extra care during a de-XUL transition.

For the record, here's a first-pass semi-trivial patch that I came up with with bgrins back in June -- this just makes us treat -moz-box like -webkit-box.
We need something a bit more subtle, though. On its own, this patch triggers assertion failures followed by a crash:
{
###!!! ASSERTION: Must be a box frame!: '!mScrollCornerBox ||
mScrollCornerBox->IsXULBoxFrame()', file
layout/generic/nsGfxScrollFrame.cpp, line 5542
###!!! ASSERTION: A box layout method was called but InitBoxMetrics was
never called: 'metrics', file layout/generic/nsFrame.cpp, line 10199
}
As I recall, this was from scroll buttons, or scroll corners, or something like that (which use display:-moz-box). A legit prototype patch might need to exclude those by giving them a special opt-out (maybe via another "-moz-box-prettyplease" display type).

Summary: Create a prototype "emulate -moz-box with modern flexbox" patch, to help see how our UI breaks with modern flexbox → Create a prototype "emulate -moz-box with modern flexbox" patch, to help prototype/discover any needed frontend changes for modern flexbox

I can do some debugging on the frontend with the Browser Toolbox next week.
Note: since the layout changes mess up the Browser Toolbox as well doing this requires two builds:
On the one with the patch applied: ./mach run --start-debugger-server 6080
On the one without the patch applied: MOZ_BROWSER_TOOLBOX_PORT=6080 ./mach run --profile /tmp/debugging -chrome chrome://devtools/content/framework/toolbox-process-window.xul

This is an updated version of the change I used for testing css flex a couple of years ago. The UI mostly appears correctly, although the content no longer does. (The content area appears correctly in the older version).
The minute long delay no longer occurs. Css flex takes 100ms to layout the Firefox UI however xul flex takes only 20ms.

Fixed up the tab bar some more. Some of the known issues remaining:
* Hamburger menu width expands to be much larger than content. So does URL bar (only sometimes while typing)
* Can't resize devtools or sidebar
* Bookmarks sidebar content is shrunk
* Adding tabs feels pretty slow, especially when holding down ctrl+t
* The tab-drop-indicator seems to take up space when it doesn't in a normal build. Maybe this is somehow related to `visibility: collapsed` on the <image> inside of the hbox.
* In OSX, tabs get moved too close to the window buttons. Maybe something with .titlebar-placeholder

Notes on the obvious visual differences there:
- Gray stripe at top of window: that is <toolbar type="menubar" id="toolbar-menubar">, which has 2px of vertical padding (1px on top and 1px on bottom), from a toolbar[type="menubar"] rule in toolbar.css. If you get rid of that padding, it disappears.
- Awkward space at left edge of tab-bar: that is the container for the drag-n-drop-tabs-between-windows "arrow" icon, <hbox class="tab-drop-indicator-box">. It has "visibility:collapse", which in XUL makes it completely collapse away; but in modern-flexbox, that just makes it collapse in the cross dimension (vertically). We need to give it "width: 0px" to collapse it in the horizontal dimension.
(This applies to any element that we expect to be collapsed away via "visibility:collapse". If it turns out there are a lot of these, then maybe we should consider a C++-level hackaround for this. But for now, a selector on the [collapsed=true] attribute might be sufficient.)
- Awkward space at left edge of URL bar: this is <box id="identity-box">, which contains some images that are all wanting to be collapsed via "visibility:collapse" via a "connection-icon, #extension-icon { visibility:collapse}" rule in browser.css. As above -- in XUL, visibility:collapse will make these 0-width, but in modern flexbox, it only makes them 0 height. We can work around this by adding "width:0" to that same CSS rule, and then overriding that in the rule that gives them "visibility:visible" (which seems to be a #urlbar[pageproxystate="valid"] selector).

(In reply to Daniel Holbert [:dholbert] from comment #19)
> (This applies to any element that we expect to be collapsed away via
> "visibility:collapse". If it turns out there are a lot of these, then maybe
> we should consider a C++-level hackaround for this. But for now, a selector
> on the [collapsed=true] attribute might be sufficient.)
Adding that style definitely makes sense as a starting point, but there are also lots of places where we use `visibility: collapse` in CSS without the attribute: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#121. So I think we'll also need to find/replace `visibility: collapse` with `visibility: collapse; width: 0` in CSS.
I'm curious:
1) how hard it would be to hack around this visibility issue in C++ without breaking web content (which I assume means only apply the hack to elements that are using the emulated -moz-box).
2) how comfortable you'd be with landing the emulation (with or without (1)) behind an off-by-default pref, assuming that we could fix obvious remaining layout issues on the frontend

(In reply to Brian Grinstead [:bgrins] from comment #22)
> So I think we'll also need to find/replace `visibility: collapse`
> with `visibility: collapse; width: 0` in CSS.
We should probably use display: none where possible.

(In reply to Dão Gottwald [::dao] from comment #23)
> (In reply to Brian Grinstead [:bgrins] from comment #22)
> > So I think we'll also need to find/replace `visibility: collapse`
> > with `visibility: collapse; width: 0` in CSS.
>
> We should probably use display: none where possible.
Do you know why we used `visibility: collapse` in the first place?

(In reply to Brian Grinstead [:bgrins] from comment #22)
> I'm curious:
> 1) how hard it would be to hack around this visibility issue in C++ without
> breaking web content (which I assume means only apply the hack to elements
> that are using the emulated -moz-box).
Not that hard. I'm working on a patch to do that right now.
> 2) how comfortable you'd be with landing the emulation (with or without (1))
> behind an off-by-default pref, assuming that we could fix obvious remaining
> layout issues on the frontend
On the C++ side, I think I'd be comfortable with it, though I'd want to clean things up slightly & improve documentation before doing that. (I'm not sure how easy it is to put the rest of the stuff in this patch behind a pref -- the .css file changes etc. -- but I assume you've got a plan for that. Or maybe you're saying that part wouldn't land just yet?)
(In reply to Brian Grinstead [:bgrins] from comment #24)
> (In reply to Dão Gottwald [::dao] from comment #23)
> > We should probably use display: none where possible.
>
> Do you know why we used `visibility: collapse` in the first place?
I think "visibility:collapse" is a bit faster to toggle than "display:none". The idea behind "visibility:collapse" (in XUL) is to make the frame 0-sized, whereas display:none will completely destroy the frame (and its descendants), and force it to be reconstructed when the display value changes to something other than "none".
So for pieces of UI that get hidden-shown somewhat frequently -- particularly the roots of deep trees which might be expensive to reconstruct -- it's probably a good idea to keep using visibility:collapse or something like it. But for infrequently-shown things (particularly things without many descendants), display:none might be a better fit.

(In reply to Brian Grinstead [:bgrins] from comment #20)
> 1) Text not wrapping in panels (seen in 03_noLWT_http):
Or rather, panels being sized to a larger default size, based on the preferred (non-wrapping) size of the text.
In current mozilla-central, this panel size comes from .panel-mainview, which has "min-width: 30em; max-width: 30em" which *is* sufficient to produce a 30em-wide panel in current mozilal-central, but isn't sufficient with the emulation patch applied.
But it works if I replace "max-width" with just "width", here:
https://hg.mozilla.org/mozilla-central/annotate/a124f4901430f6db74cfc7fe3b07957a1c691b40/browser/themes/shared/customizableui/panelUI.inc.css#l351
I don't know what's going on here -- this is a child of a XUL stack, and I don't know how XUL stack layout works really. This is probably an issue with us violating some expected invariant at the XUL-parent --> nonXUL-child (stack->flex) boundary.
> 2) Pinned tab layout issues
This is just a bug in the current patch, I think. Right now, the patch adds "width: 0" on .tab-stack in browser/themes/shared/tabs.inc.css. If you remove that new line from the patch, then pinned tabs work fine for me (and nothing obviously gets worse).

This patch is probably the correct way to hide the menubar "stripe" (the first thing noted in comment 19). Feel free to merge it into the current megapatch and mark this attachment as obsolete.
I'm guessing legitimate-XUL (in current mozilla-central) squashes the (nonzero) padding down to 0, due to having "height:0" and "box-sizing:border-box" on this element, or something like that. But with HTML/CSS, that isn't sufficient to squash the padding away, as shown by the presence of green in this data URL:
data:text/html,<!doctype html><div style="background:green;padding:10px;height:0;box-sizing:border-box">
So we have to actually set the menubar's padding to 0 in these cases where we're clearing out the height and the border.

(In reply to Daniel Holbert [:dholbert] from comment #26)
> > 2) Pinned tab layout issues
>
> This is just a bug in the current patch, I think. Right now, the patch adds
> "width: 0" on .tab-stack in browser/themes/shared/tabs.inc.css.
(Oh, it looks like maybe this hack was necessary to prevent normal-tabs from overlapping each other... if so, maybe we do need it after all -- or maybe we need something more targeted that doesn't impact pinned tabs.)

(In reply to Daniel Holbert [:dholbert] from comment #28)
> (In reply to Daniel Holbert [:dholbert] from comment #26)
> > > 2) Pinned tab layout issues
> >
> > This is just a bug in the current patch, I think. Right now, the patch adds
> > "width: 0" on .tab-stack in browser/themes/shared/tabs.inc.css.
>
> (Oh, it looks like maybe this hack was necessary to prevent normal-tabs from
> overlapping each other... if so, maybe we do need it after all -- or maybe
> we need something more targeted that doesn't impact pinned tabs.)
Yeah, it was added to prevent overlapping - there some some issue I think with the <stack class="tab-stack"> and it's ::after content which actually draws the border. I expect we can make something more targeted to prevent messing up pinned tabs

(In reply to Daniel Holbert [:dholbert] from comment #25)
> (In reply to Brian Grinstead [:bgrins] from comment #24)
> > (In reply to Dão Gottwald [::dao] from comment #23)
> > > We should probably use display: none where possible.
> >
> > Do you know why we used `visibility: collapse` in the first place?
>
> I think "visibility:collapse" is a bit faster to toggle than "display:none".
> The idea behind "visibility:collapse" (in XUL) is to make the frame 0-sized,
> whereas display:none will completely destroy the frame (and its
> descendants), and force it to be reconstructed when the display value
> changes to something other than "none".
>
> So for pieces of UI that get hidden-shown somewhat frequently --
> particularly the roots of deep trees which might be expensive to reconstruct
> -- it's probably a good idea to keep using visibility:collapse or something
> like it. But for infrequently-shown things (particularly things without
> many descendants), display:none might be a better fit.
We also use it to keep XBL bindings attached e.g. when hiding the tab strip in popup windows.

Daniel and I looked closer at this and figured out why the panel height is changing:
In XUL flex there was an issue with the contents of the permission section which was worked around by adding extra padding on #identity-popup-permissions-content in Bug 1368281.
It appears this was needed because the min-height on the #identity-popup-permissions-headline did not cause the parent height to expand (although it did push down sibling text for the permission label): https://hg.mozilla.org/mozilla-central/annotate/a97fb1c39d51a7337b46957bde4273bd308b796a/browser/themes/shared/controlcenter/panel.inc.css#l369.
With CSS flex the min-height does apply - you can see this by changing the property in the inspector and see that the panel actually expands, while in the current XUL flex world it doesn't. The CSS flex behavior seems strictly better, so I'd suggest we get rid of the extra .5 padding-bottom on the panel and live with a couple of px difference (since now the 24px min-height is causing the panel to expand just a bit extra to accommodate it's child height)

(In reply to Daniel Holbert [:dholbert] from comment #25)
> (In reply to Brian Grinstead [:bgrins] from comment #22)
> > I'm curious:
> > 1) how hard it would be to hack around this visibility issue in C++ without
> > breaking web content (which I assume means only apply the hack to elements
> > that are using the emulated -moz-box).
>
> Not that hard. I'm working on a patch to do that right now.
>
> > 2) how comfortable you'd be with landing the emulation (with or without (1))
> > behind an off-by-default pref, assuming that we could fix obvious remaining
> > layout issues on the frontend
>
> On the C++ side, I think I'd be comfortable with it, though I'd want to
> clean things up slightly & improve documentation before doing that. (I'm
> not sure how easy it is to put the rest of the stuff in this patch behind a
> pref -- the .css file changes etc. -- but I assume you've got a plan for
> that. Or maybe you're saying that part wouldn't land just yet?)
-moz-bool-pref may be the best option for landing the CSS changes. Although we could also land the platform changes in this bug and then file follow up(s) to land the CSS/JS changes (and either block that work on Bug 1267890 or come up with another solution there).

Permalink for comment 41:
https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/layout/base/nsCSSFrameConstructor.cpp#10848-10849
[6713, Main Thread] ###!!! ASSERTION: frame wants to be inside an anonymous item, but it isn't: '!FrameWantsToBeInAnonymousItem(aParentFrame, child)', file layout/base/nsCSSFrameConstructor.cpp, line 10863
That's something we need to eventually address -- but at worst, it means we'll have incorrect positions, probably not crashes or anything like that.
I looked at it briefly, and it looks like "child" is a nsPlaceholderFrame for a nsMenuFrame. nsMenuFrame violates our expectations here because it's out-of-flow despite having the default value for 'position'. (It gets "aIsOutOfFlowPopup" set in some nsCSSFrameConstructor methods, gets a placeholder as a result.)
We probably need to just adjust NeedsAnonFlexOrGridItem to check for "|| mIsPopup" alongside its existing aState.GetGeometricParent() check.

I'll be posting a C++ patch-stack for review here pretty soon (basically the c++ bits of the current patch, split into modular pieces, with comment 41-42 addressed). The non-C++ changes from the current patch can continue to be developed over on bug 1424095.

To be clear, this is a "paving the way" patch. At this point in the patch
series, it's not yet possible for us to generate a nsFlexContainerFrame that
has display:-moz-box. (A later patch in this series will make that possible.)
This patch adds the mechanics to nsFlexContainerFrame instances so that they'll
label themselves appropriately (with NS_STATE_FLEX_IS_EMULATING_LEGACY_BOX)
once it *does* become possible for -moz-box to spawn a nsFlexContainerFrame.
Moreover, this patch updates the state bit's documentation to reflect its new
potential-usage.
Review commit: https://reviewboard.mozilla.org/r/207556/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207556/

In modern flexbox and in "display:-webkit-box", children with
"visibility:collapse" currently generate "struts" which have 0 main-size but
nonzero cross-size.
But XUL/-moz-box treats these children differently -- it makes them 0-sized in
both axes. So we need to add a custom behavior to modern flexbox in order to
emulate that.
Specifically, this patch makes us:
- Ignore these children when computing the flex container's intrinsic sizes.
- Take a simpler codepath with 0-sized struts for collapsed elements when
laying out a -moz-box (rather than the typical 2-pass layout, with strut
cross-size being established in the 1st pass).
Review commit: https://reviewboard.mozilla.org/r/207564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207564/

Comment on attachment 8936861[details]Bug 1398963 part 6: Make "visibility:collapse" cause flex items to be 0-sized, in emulated -moz-{inline-}box.
https://reviewboard.mozilla.org/r/207564/#review213828> nit: perhaps add MOZ_ASSERT(aFlexContainer->StyleDisplay() == aFlexStyleDisp) at the start to make it clear the 2nd param is merely intended as an optimization?
>
> Also, the first arg is always |this| so perhaps this function should be a nsFlexContainerFrame method instead?
Agreed on both counts. Thanks!
> nit: remove the redundant parens + fits on one line?
Can't unwrap to one line, sadly. Even without the parens, it's an 82-character-long line (in all three cases).
Given that, I'll leave this as-is, because the parens are helpful IMO for making the logic clearer and distinguishing between the && vs. == operators (and for quickly visualizing the grouping). I find the parenthesized version easier to read here:
Aaaaaaaaa &&
Bbbbbbbbb ==
Ccccccccc
vs.
Aaaaaaaaa &&
(Bbbbbbbbb ==
Ccccccccc)

Comment on attachment 8936862[details]Bug 1398963 part 7: Add an about:config flag to optionally emulate -moz-box with flexbox.
https://reviewboard.mozilla.org/r/207566/#review213924
::: commit-message-f41f8:1
(Diff revision 1)
> +Bug 1398963 part 7: Add an about:config flag to optionally emulate -moz-box with modern flexbox. r?mats
Good catch RE inconsistent meanings of "modern flexbox" here (in some cases meaning nsFlexContainerFrame, in some cases meaning legit display:flex).
I clarified the earlier comment that you quoted here (from part 6) with the following:
// Quick filter to screen out *actual* (not-coopted-for-emulation)
// flex containers, using state bit:
And I'll drop "modern" throughout this patch as you suggest, for good measure. So, hopefully clearer on both sides.