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

RESOLVED FIXED in Firefox 59

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(12 attachments, 6 obsolete attachments)

47.90 KB, image/png
Details
60.40 KB, image/png
Details
30.95 KB, text/plain
Details
65.96 KB, image/png
Details
648.45 KB, image/png
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
(Assignee)

Description

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

Comment 1

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

Updated

2 years ago
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
(Assignee)

Updated

2 years ago
Flags: needinfo?(dholbert)
See Also: → bug 1183163
(Assignee)

Comment 2

2 years ago
Flags: needinfo?(dholbert)
(Assignee)

Updated

2 years ago
Attachment #8906818 - Attachment is obsolete: true
(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
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
Flags: needinfo?(bgrinstead)
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

2 years ago
Posted file 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.
:dholbert's patch plus some CSS to convert attribute values into the corresponding XUL flex properties and fix various ui issues
Flags: needinfo?(bgrinstead)
(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!
(Assignee)

Updated

a year ago
Blocks: 1033225
Attachment #8915734 - Attachment is obsolete: true
Comment hidden (mozreview-request)
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 hidden (mozreview-request)
Here's a profile when holding down cmd+t on OSX: https://perfht.ml/2la9r3L
Priority: -- → P3
(Assignee)

Comment 19

a year 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).
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
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; }
```
(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
Flags: needinfo?(dholbert)
(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?
(Assignee)

Comment 25

a year 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.
Flags: needinfo?(dholbert)
(Assignee)

Comment 26

a year 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).
(Assignee)

Comment 27

a year 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.
(Assignee)

Comment 28

a year 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.)
(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
Attachment #8913877 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8922097 - Attachment description: additional patch (fix menubar stripe, fadeout of URLbar, and visibility:collapse stuff) → additional patch (fix menubar stripe, fadeout of URLbar, extra-wide site ID panel, and visibility:collapse stuff)
Comment hidden (mozreview-request)
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)
Attachment #8922097 - Attachment is obsolete: true
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.
(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)
See Also: → bug 1368281
Comment hidden (mozreview-request)

Comment 38

a year 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.
Blocks: 1293242
(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).
Depends on: 1267890
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?
(Assignee)

Comment 42

a year 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.
(Assignee)

Updated

a year ago
Blocks: 1424095
(Assignee)

Comment 43

a year 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8921235 - Attachment is obsolete: true

Comment 51

a year 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
Attachment #8936856 - Flags: review?(mats) → review+

Comment 52

a year 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
Attachment #8936857 - Flags: review?(mats) → review+

Comment 53

a year 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
Attachment #8936858 - Flags: review?(mats) → review+

Comment 54

a year 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
Attachment #8936859 - Flags: review?(mats) → review+

Comment 55

a year 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
Attachment #8936860 - Flags: review?(mats) → review+

Comment 56

a year 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
Attachment #8936861 - Flags: review?(mats) → review+

Comment 57

a year 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

Comment 58

a year 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
Attachment #8936862 - Flags: review?(mats) → review+
(Assignee)

Comment 59

a year 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)
(Assignee)

Comment 60

a year 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 68

a year 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
status-firefox57: affected → ---
You need to log in before you can comment on or make changes to this bug.