Daniel Holbert [:dholbert] Assignee
	Description • 7 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 1 • 7 years ago

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

	Brian Grinstead [:bgrins]
	Comment 4 • 7 years ago

(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

	Brian Grinstead [:bgrins]
	Comment 6 • 7 years ago

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

	Brian Grinstead [:bgrins]
	Comment 7 • 7 years ago

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

	Neil Deakin
	Comment 8 • 7 years ago

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.

	Brian Grinstead [:bgrins]
	Comment 9 • 7 years ago

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

	Brian Grinstead [:bgrins]
	Comment 10 • 7 years ago

With Attachment 8915734 [details] [diff] applied

	Dão Gottwald [:dao]
	Comment 11 • 7 years ago

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

	Brian Grinstead [:bgrins]
	Comment 14 • 7 years ago

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

	Brian Grinstead [:bgrins]
	Comment 16 • 7 years ago

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

	Brian Grinstead [:bgrins]
	Comment 17 • 7 years ago

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

	Brian Grinstead [:bgrins]
	Comment 18 • 7 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 19 • 7 years ago

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

	Brian Grinstead [:bgrins]
	Comment 20 • 7 years ago

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

	Brian Grinstead [:bgrins]
	Comment 21 • 7 years ago

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; }
```

	Brian Grinstead [:bgrins]
	Comment 22 • 7 years ago

(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

	Dão Gottwald [:dao]
	Comment 23 • 7 years ago

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

	Brian Grinstead [:bgrins]
	Comment 24 • 7 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 25 • 7 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 26 • 7 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 27 • 7 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 28 • 7 years ago

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

	Brian Grinstead [:bgrins]
	Comment 29 • 7 years ago

(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

	Brian Grinstead [:bgrins]
	Comment 32 • 7 years ago

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)

	Brian Grinstead [:bgrins]
	Comment 33 • 7 years ago

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.

	Brian Grinstead [:bgrins]
	Comment 34 • 7 years ago

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

	Dão Gottwald [:dao]
	Comment 35 • 7 years ago

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

	Brian Grinstead [:bgrins]
	Comment 36 • 7 years ago

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)

	:Paolo Amadini
	Comment 38 • 7 years ago

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.

	Brian Grinstead [:bgrins]
	Comment 39 • 7 years ago

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

	Brian Grinstead [:bgrins]
	Comment 40 • 7 years ago

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

	Dave Townsend [:mossop]
	Comment 41 • 7 years ago

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?

	Daniel Holbert [:dholbert] Assignee
	Comment 42 • 7 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 43 • 6 years ago

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.

	Mats Palmgren (inactive)
	Comment 51 • 6 years ago
mozreview-review

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

	Mats Palmgren (inactive)
	Comment 52 • 6 years ago
mozreview-review

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

	Mats Palmgren (inactive)
	Comment 53 • 6 years ago
mozreview-review

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

	Mats Palmgren (inactive)
	Comment 54 • 6 years ago
mozreview-review

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

	Mats Palmgren (inactive)
	Comment 55 • 6 years ago
mozreview-review

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

	Mats Palmgren (inactive)
	Comment 56 • 6 years ago
mozreview-review

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

	Mats Palmgren (inactive)
	Comment 57 • 6 years ago
mozreview-review

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

	Mats Palmgren (inactive)
	Comment 58 • 6 years ago
mozreview-review

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

	Daniel Holbert [:dholbert] Assignee
	Comment 59 • 6 years ago
mozreview-review-reply

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)

	Daniel Holbert [:dholbert] Assignee
	Comment 60 • 6 years ago
mozreview-review

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.

	Pulsebot
	Comment 68 • 6 years ago

Pushed by dholbert@mozilla.com:
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

	Dorel Luca [:dluca]
	Comment 69 • 6 years ago
bugherder

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