1398963 - Create a prototype "emulate -moz-box with modern flexbox" patch, to help prototype/discover any needed frontend changes for modern flexbox

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: strawman patch v1 (asserts/crashes)

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

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: patch v1 (doesn't crash, partially-functional UI appears)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: screenshot of Firefox window (at first run) with patch v1

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Created attachment 8913878 [details]
> screenshot of Firefox window (at first run) with patch v1

Could be worse! Thanks

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: osx-first-run-v1.png

Comment 6

[Brian Grinstead [:bgrins]]

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

Comment 7

[Brian Grinstead [:bgrins]]

Just did a quick check and a couple of notes:

1) it appears the area for web page contents which isn't expanding is inside a <deck> (which has display: -moz-deck)
2) the urlbar has a 1px width: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#540-546 and when that gets removed, it expands as expected

Comment 8

[Neil Deakin]

Attachment: css flex change

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.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: moz-box-webkit-box-with-frontend-WIP.patch

:dholbert's patch plus some CSS to convert attribute values into the corresponding XUL flex properties and fix various ui issues

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: osx-with-8915734.png

With Attachment 8915734 [details] [diff] applied

Comment 11

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Created attachment 8915738 [details]
> osx-with-8915734.png
> 
> With Attachment 8915734 [details] [diff] applied

Looks very promising!

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: Bug 1398963 - Create a prototype "emulate -moz-box with modern flexbox" patch, to help prototype/discover any needed frontend changes for modern flexbox

Review commit: https://reviewboard.mozilla.org/r/192236/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192236/

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8921235 [details]
Bug 1398963 - Create a prototype "emulate -moz-box with modern flexbox" patch, to help prototype/discover any needed frontend changes for modern flexbox

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192236/diff/1-2/

Comment 14

[Brian Grinstead [:bgrins]]

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

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8921235 [details]
Bug 1398963 - Create a prototype "emulate -moz-box with modern flexbox" patch, to help prototype/discover any needed frontend changes for modern flexbox

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192236/diff/2-3/

Comment 16

[Brian Grinstead [:bgrins]]

Getting some talos results in: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=c0940c698bc3908463f9f8c9a702edd361cab9db&newProject=try&newRevision=5a0e9cdc2d5651ee1fdf8639a96884084ee2ab8a&framework=1&showOnlyImportant=1

Comment 17

[Brian Grinstead [:bgrins]]

Here's a profile when holding down cmd+t on OSX: https://perfht.ml/2la9r3L

Comment 18

[Brian Grinstead [:bgrins]]

Mozscreenshots comparison: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=967c95cee709756596860ed2a3e6ac06ea3a053f&newProject=try&newRev=c853f4399b154d6e5730cb3862a04615fc957a97

Comment 19

[Daniel Holbert [:dholbert]]

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

Comment 20

[Brian Grinstead [:bgrins]]

Thanks Daniel, if we can address those main points from CSS we can hopefully get a more targeted compare UI with mozscreenshots since all screenshots are showing up as different due to them.

A couple of other issues I noticed:

1) Text not wrapping in panels (seen in 03_noLWT_http):
- Current behavior (has wrapping): https://public-artifacts.taskcluster.net/BPY9AXk8RZOTdKqAMkcvQg/0/public/test_info//20171024132119-controlCenter_03_noLWT_http.png
- With patch (no wrapping): https://public-artifacts.taskcluster.net/MVSz6tLIQwSTccD446zZTg/0/public/test_info//20171024201647-controlCenter_03_noLWT_http.png

Interesting that text *does* wrap in certain cases (i.e. 16_darkLWT_persistentStorage): https://public-artifacts.taskcluster.net/MVSz6tLIQwSTccD446zZTg/0/public/test_info//20171024201647-permissionPrompts_16_darkLWT_persistentStorage.png

2) Pinned tab layout issues (091_tabsOutsideTitlebar_twoPinnedWithOverflow_maximized_onlyNavBar_noLWT):
- Current behavior: https://public-artifacts.taskcluster.net/BPY9AXk8RZOTdKqAMkcvQg/0/public/test_info//20171024132119-primaryUI_091_tabsOutsideTitlebar_twoPinnedWithOverflow_maximized_onlyNavBar_noLWT.png
- With patch (pinned tabs pushed to left of viewport): https://public-artifacts.taskcluster.net/MVSz6tLIQwSTccD446zZTg/0/public/test_info//20171024201647-primaryUI_091_tabsOutsideTitlebar_twoPinnedWithOverflow_maximized_onlyNavBar_noLWT.png

Comment 21

[Brian Grinstead [:bgrins]]

Having the attr() CSS function (bug 435426) would let us not do this in CSS:

```
[flex="1"] { -moz-box-flex: 1; }
[flex="2"] { -moz-box-flex: 2; }
...
[flex="1000"] { -moz-box-flex: 1000; }
```

Comment 22

[Brian Grinstead [:bgrins]]

(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

Comment 23

[Dão Gottwald [:dao]]

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

Comment 24

[Brian Grinstead [:bgrins]]

(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?

Comment 25

[Daniel Holbert [:dholbert]]

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

Comment 26

[Daniel Holbert [:dholbert]]

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

Comment 27

[Daniel Holbert [:dholbert]]

Attachment: patch to hide 2px-tall menubar stripe

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.

Comment 28

[Daniel Holbert [:dholbert]]

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

Comment 29

[Brian Grinstead [:bgrins]]

(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

Comment 30

[Daniel Holbert [:dholbert]]

Attachment: additional patch (fix menubar stripe, fadeout of URLbar, extra-wide site ID panel, and visibility:collapse stuff)

Comment 31

[Brian Grinstead [:bgrins]]

Comment on attachment 8921235 [details]
Bug 1398963 - Create a prototype "emulate -moz-box with modern flexbox" patch, to help prototype/discover any needed frontend changes for modern flexbox

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192236/diff/3-4/

Comment 32

[Brian Grinstead [:bgrins]]

Comment on attachment 8922097 [details] [diff] [review]
additional patch (fix menubar stripe, fadeout of URLbar, extra-wide site ID panel, and visibility:collapse stuff)

Folded this into the mozreview attachment (along with a titlebar spacing fix for OSX)

Comment 33

[Brian Grinstead [:bgrins]]

New mozscreenshot run: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=967c95cee709756596860ed2a3e6ac06ea3a053f&newProject=try&newRev=dbfb48bcd34b5ade3ef39e1b9c8923e2994a2c52.

Much better with the fixes! On Linux (where the compare job has finished) we get 100 identical and 27 differences. And if we take away the known pinned tabs and devtools differences it looks like we are down to just 2 (01_noLWT_about and 01_prefsGeneral).

Note that there's a bug in the mozscreenshot UI that makes the difference images show up as 404, but you can click 'base' and 'new' and flip between them to see the differences.

Comment 34

[Brian Grinstead [:bgrins]]

On OSX we are getting a lot of diffs due to a bit of extra height on the identity popup:

- Without patch: https://public-artifacts.taskcluster.net/ERNh3JaOTZOz0_YmQjTlSQ/0/public/test_info/20171024132119-controlCenter_11_noLWT_mixedPassive.png
- With patch: https://public-artifacts.taskcluster.net/RpFIvkLlQnGWWj7WkCx6ew/0/public/test_info/20171025223203-controlCenter_11_noLWT_mixedPassive.png

Comment 35

[Dão Gottwald [:dao]]

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

Comment 36

[Brian Grinstead [:bgrins]]

Attachment: xul-left-css-right.png

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)

Comment 37

[Brian Grinstead [:bgrins]]

Comment on attachment 8921235 [details]
Bug 1398963 - Create a prototype "emulate -moz-box with modern flexbox" patch, to help prototype/discover any needed frontend changes for modern flexbox

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192236/diff/4-5/

Comment 38

[:Paolo Amadini]

Also see bug 1293242 and the descriptionHeightWorkaround function:

https://dxr.mozilla.org/mozilla-central/search?q=descriptionHeightWorkaround&redirect=true

Hopefully, in this new world this workaround won't be needed anymore. Since stack layout was also mentioned, I'll note that in the new Photon panels we still have a stack element, but I don't think we're really depending on its layout features in practice, so it could easily be replaced with a normal box.

Comment 39

[Brian Grinstead [:bgrins]]

New mozscreenshots run: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=967c95cee709756596860ed2a3e6ac06ea3a053f&newProject=try&newRev=b4cff6a2492b96a00dd0f5a0f59e16eb8bf54b9f

Notes:
- the panel height still shows up changed across all platforms (which I think is OK as mentioned in Comment 36)
-- https://screenshots.mattn.ca/comparisons/mozilla-central/967c95cee709756596860ed2a3e6ac06ea3a053f/try/b4cff6a2492b96a00dd0f5a0f59e16eb8bf54b9f/linux64/controlCenter_03_noLWT_http.png
- iframe heights are still broken, need to investigate that
-- https://screenshots.mattn.ca/comparisons/mozilla-central/967c95cee709756596860ed2a3e6ac06ea3a053f/try/b4cff6a2492b96a00dd0f5a0f59e16eb8bf54b9f/linux64/devtools_1_bottomToolbox.png
- Windows has some missing height above the tabs
-- 10: https://screenshots.mattn.ca/comparisons/mozilla-central/967c95cee709756596860ed2a3e6ac06ea3a053f/try/b4cff6a2492b96a00dd0f5a0f59e16eb8bf54b9f/windows10-64/controlCenter_01_noLWT_about.png
-- 7: https://screenshots.mattn.ca/comparisons/mozilla-central/967c95cee709756596860ed2a3e6ac06ea3a053f/try/b4cff6a2492b96a00dd0f5a0f59e16eb8bf54b9f/windows7-32/controlCenter_22_darkLWT_about.png

Comment 40

[Brian Grinstead [:bgrins]]

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

Comment 41

[Dave Townsend [:mossop]]

I see a lot of assertions in debug builds with this patch applied (https://searchfox.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#10908), is that something we need to worry about?

Comment 42

[Daniel Holbert [:dholbert]]

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.

Comment 43

[Daniel Holbert [:dholbert]]

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.

Comment 44

[Daniel Holbert [:dholbert]]

Attachment: Bug 1398963 part 1: Rename frame-state bit from NS_STATE_FLEX_IS_LEGACY_WEBKIT_BOX to NS_STATE_FLEX_IS_EMULATING_LEGACY_BOX (idempotent patch).

This patch isn't changing semantics of this bit at all - it just renames it to
a more general name. In other words, this patch does not change behavior.

Review commit: https://reviewboard.mozilla.org/r/207554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207554/

Comment 45

[Daniel Holbert [:dholbert]]

Attachment: Bug 1398963 part 2: Make nsFlexContainerFrame label itself as legacy if it has -moz-box/-moz-inline-box display val.

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/

Comment 46

[Daniel Holbert [:dholbert]]

Attachment: Bug 1398963 part 3: Generalize "webkit-box" variable names and code-comments in nsCSSFrameConstructor.cpp (idempotent patch).

Now that (per previous patch) NS_STATE_FLEX_IS_EMULATING_LEGACY_BOX isn't just
a webkit-box-specific tag: this patch here generalizes the variable-names and
comments associated with that flag in nsCSSFrameConstructor.cpp.

This patch does not make any changes to behavior; it's simply renaming &
comment tweaks.

Review commit: https://reviewboard.mozilla.org/r/207558/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207558/

Comment 47

[Daniel Holbert [:dholbert]]

Attachment: Bug 1398963 part 4: Refactor logic in nsCSSFrameConstructor's IsXULDisplayType() function (idempotent patch).

This patch does not change behavior. This patch just refactors the logic in
IsXULDisplayType so that -moz-box and -moz-inline-box are handled via a
dedicated early-return.  This lets a later patch in this series make a more
understandable targeted tweak to add pref-controlled behavior for these display
values.

Review commit: https://reviewboard.mozilla.org/r/207560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207560/

Comment 48

[Daniel Holbert [:dholbert]]

Attachment: Bug 1398963 part 5: Treat XUL Popups like other OOF boxes when generating anon flex items, since they spawn placeholders.

UL popups (i.e. FrameConstructionItem instances with mIsPopup==true) behave
like out-of-flow content -- in particular, they generate nsPlaceholderFrame
instances.  So, they need the same placeholder-wrapping behavior that we have
for other out-of-flow frames inside of an emulated legacy box, in order to
satisfy our existing invariants.

Review commit: https://reviewboard.mozilla.org/r/207562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207562/

Comment 49

[Daniel Holbert [:dholbert]]

Attachment: Bug 1398963 part 6: Make "visibility:collapse" cause flex items to be 0-sized, in emulated -moz-{inline-}box.

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 50

[Daniel Holbert [:dholbert]]

Attachment: Bug 1398963 part 7: Add an about:config flag to optionally emulate -moz-box with flexbox.

This feature is intended to help Firefox frontend developers experiment with
replacing XUL content with modern flexbox.  We might also eventually use
this emulation to *actually* render most or all of our legacy XUL UI.

Review commit: https://reviewboard.mozilla.org/r/207566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207566/

Comment 51

[Mats Palmgren (inactive)]

Comment on attachment 8936856 [details]
Bug 1398963 part 1: Rename frame-state bit from NS_STATE_FLEX_IS_LEGACY_WEBKIT_BOX to NS_STATE_FLEX_IS_EMULATING_LEGACY_BOX (idempotent patch).

https://reviewboard.mozilla.org/r/207554/#review213818

Comment 52

[Mats Palmgren (inactive)]

Comment on attachment 8936857 [details]
Bug 1398963 part 2: Make nsFlexContainerFrame label itself as legacy if it has -moz-box/-moz-inline-box display val.

https://reviewboard.mozilla.org/r/207556/#review213820

Comment 53

[Mats Palmgren (inactive)]

Comment on attachment 8936858 [details]
Bug 1398963 part 3: Generalize "webkit-box" variable names and code-comments in nsCSSFrameConstructor.cpp (idempotent patch).

https://reviewboard.mozilla.org/r/207558/#review213822

Comment 54

[Mats Palmgren (inactive)]

Comment on attachment 8936859 [details]
Bug 1398963 part 4: Refactor logic in nsCSSFrameConstructor's IsXULDisplayType() function (idempotent patch).

https://reviewboard.mozilla.org/r/207560/#review213824

Comment 55

[Mats Palmgren (inactive)]

Comment on attachment 8936860 [details]
Bug 1398963 part 5: Treat XUL Popups like other OOF boxes when generating anon flex items, since they spawn placeholders.

https://reviewboard.mozilla.org/r/207562/#review213826

Comment 56

[Mats Palmgren (inactive)]

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

::: layout/generic/nsFlexContainerFrame.cpp:111
(Diff revision 1)
> +ShouldUseMozBoxCollapseBehavior(const nsIFrame* aFlexContainer,
> +                                const nsStyleDisplay* aFlexStyleDisp)

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?

::: layout/generic/nsFlexContainerFrame.cpp:3740
(Diff revision 1)
> +        (NS_STYLE_VISIBILITY_COLLAPSE ==
> +         childFrame->StyleVisibility()->mVisible)) {

nit: remove the redundant parens + fits on one line?

::: layout/generic/nsFlexContainerFrame.cpp:4972
(Diff revision 1)
> +        (NS_STYLE_VISIBILITY_COLLAPSE !=
> +         childFrame->StyleVisibility()->mVisible)) {

ditto

::: layout/generic/nsFlexContainerFrame.cpp:5015
(Diff revision 1)
> +        (NS_STYLE_VISIBILITY_COLLAPSE !=
> +         childFrame->StyleVisibility()->mVisible)) {

ditto

Comment 57

[Mats Palmgren (inactive)]

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/#review213832

::: 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

"emulate -moz-box with modern flexbox" sounds a bit odd since an earlier patch said:
  // Quick filter to screen out modern-flexbox using state bit:
  if (\!IsLegacyBox(aFlexContainer)) \{
so I gather when the frame state bit is true then it's NOT a "modern flexbox", however
when we emulate emulate -moz-box using a nsFlexContainerFrame that bit is true,
so technically we're emulating -moz-box using a non-modern flexbox.

Perhaps just drop "modern" to avoid the issue? :-)

::: layout/base/nsCSSFrameConstructor.cpp:4403
(Diff revision 1)
>  }
>  
>  static
>  bool IsXULDisplayType(const nsStyleDisplay* aDisplay)
>  {
> -  if  (aDisplay->mDisplay == StyleDisplay::MozInlineBox ||
> +  // -moz-{inline-}box is XUL, unless we're emulating it with modern flexbox.

ditto

::: layout/base/nsCSSFrameConstructor.cpp:4656
(Diff revision 1)
>  
>    if (aDisplay->mDisplay < StyleDisplay::MozBox) {
>      return nullptr;
>    }
>  
> +  // If we're emulating -moz-box with modern flexbox, then treat it as non-XUL

ditto

::: modules/libpref/init/all.js:2974
(Diff revision 1)
>  pref("layout.css.frames-timing.enabled", false);
>  #else
>  pref("layout.css.frames-timing.enabled", true);
>  #endif
>  
> +// Are we emulating -moz-{inline}-box layout using modern CSS flexbox?

ditto

Comment 58

[Mats Palmgren (inactive)]

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/#review213834

Comment 59

[Daniel Holbert [:dholbert]]

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 60

[Daniel Holbert [:dholbert]]

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.

Comment 61

[Daniel Holbert [:dholbert]]

Comment on attachment 8936856 [details]
Bug 1398963 part 1: Rename frame-state bit from NS_STATE_FLEX_IS_LEGACY_WEBKIT_BOX to NS_STATE_FLEX_IS_EMULATING_LEGACY_BOX (idempotent patch).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207554/diff/1-2/

Comment 62

[Daniel Holbert [:dholbert]]

Comment on attachment 8936857 [details]
Bug 1398963 part 2: Make nsFlexContainerFrame label itself as legacy if it has -moz-box/-moz-inline-box display val.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207556/diff/1-2/

Comment 63

[Daniel Holbert [:dholbert]]

Comment on attachment 8936858 [details]
Bug 1398963 part 3: Generalize "webkit-box" variable names and code-comments in nsCSSFrameConstructor.cpp (idempotent patch).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207558/diff/1-2/

Comment 64

[Daniel Holbert [:dholbert]]

Comment on attachment 8936859 [details]
Bug 1398963 part 4: Refactor logic in nsCSSFrameConstructor's IsXULDisplayType() function (idempotent patch).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207560/diff/1-2/

Comment 65

[Daniel Holbert [:dholbert]]

Comment on attachment 8936860 [details]
Bug 1398963 part 5: Treat XUL Popups like other OOF boxes when generating anon flex items, since they spawn placeholders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207562/diff/1-2/

Comment 66

[Daniel Holbert [:dholbert]]

Comment on attachment 8936861 [details]
Bug 1398963 part 6: Make "visibility:collapse" cause flex items to be 0-sized, in emulated -moz-{inline-}box.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207564/diff/1-2/

Comment 67

[Daniel Holbert [:dholbert]]

Comment on attachment 8936862 [details]
Bug 1398963 part 7: Add an about:config flag to optionally emulate -moz-box with flexbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207566/diff/1-2/

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/959ac9840798
part 1: Rename frame-state bit from NS_STATE_FLEX_IS_LEGACY_WEBKIT_BOX to NS_STATE_FLEX_IS_EMULATING_LEGACY_BOX (idempotent patch). r=mats
https://hg.mozilla.org/integration/autoland/rev/462e7346039f
part 2: Make nsFlexContainerFrame label itself as legacy if it has -moz-box/-moz-inline-box display val. r=mats
https://hg.mozilla.org/integration/autoland/rev/39f1f91f7b0b
part 3: Generalize "webkit-box" variable names and code-comments in nsCSSFrameConstructor.cpp (idempotent patch). r=mats
https://hg.mozilla.org/integration/autoland/rev/2d4dd2c2712e
part 4: Refactor logic in nsCSSFrameConstructor's IsXULDisplayType() function (idempotent patch). r=mats
https://hg.mozilla.org/integration/autoland/rev/9d1204867a51
part 5: Treat XUL Popups like other OOF boxes when generating anon flex items, since they spawn placeholders. r=mats
https://hg.mozilla.org/integration/autoland/rev/16e76fada893
part 6: Make "visibility:collapse" cause flex items to be 0-sized, in emulated -moz-{inline-}box. r=mats
https://hg.mozilla.org/integration/autoland/rev/4f4836d4a084
part 7: Add an about:config flag to optionally emulate -moz-box with flexbox. r=mats

Comment 69

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/959ac9840798
https://hg.mozilla.org/mozilla-central/rev/462e7346039f
https://hg.mozilla.org/mozilla-central/rev/39f1f91f7b0b
https://hg.mozilla.org/mozilla-central/rev/2d4dd2c2712e
https://hg.mozilla.org/mozilla-central/rev/9d1204867a51
https://hg.mozilla.org/mozilla-central/rev/16e76fada893
https://hg.mozilla.org/mozilla-central/rev/4f4836d4a084