Closed
Bug 1398963
Opened 7 years ago
Closed 7 years ago
Create a prototype "emulate -moz-box with modern flexbox" patch, to help prototype/discover any needed frontend changes for modern flexbox
Categories
(Core :: Layout, enhancement, P3)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(12 files, 6 obsolete files)
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
|
MatsPalmgren_bugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
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•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).
Assignee | ||
Updated•7 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•7 years ago
|
Flags: needinfo?(dholbert)
Assignee | ||
Comment 2•7 years ago
|
||
Flags: needinfo?(dholbert)
Assignee | ||
Updated•7 years ago
|
Attachment #8906818 -
Attachment is obsolete: true
Assignee | ||
Comment 3•7 years ago
|
||
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
Comment 5•7 years ago
|
||
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
Flags: needinfo?(bgrinstead)
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
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.
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
Flags: needinfo?(bgrinstead)
Comment 10•7 years ago
|
||
With Attachment 8915734 [details] [diff] applied
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!
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8915734 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
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
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
Comment 17•7 years ago
|
||
Here's a profile when holding down cmd+t on OSX: https://perfht.ml/2la9r3L
Updated•7 years ago
|
Priority: -- → P3
Comment 18•7 years ago
|
||
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).
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
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; }
```
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
Flags: needinfo?(dholbert)
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.
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?
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.
Flags: needinfo?(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).
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.
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.)
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
Updated•7 years ago
|
Attachment #8913877 -
Attachment is obsolete: true
Assignee | ||
Comment 30•7 years ago
|
||
Attachment #8922067 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years 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 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)
Attachment #8922097 -
Attachment is obsolete: true
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.
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
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.
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)
Comment hidden (mozreview-request) |
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.
Blocks: 1293242
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
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).
Depends on: 1267890
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?
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.
Assignee | ||
Comment 43•7 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.
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•7 years ago
|
Attachment #8921235 -
Attachment is obsolete: true
Comment 51•7 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
Attachment #8936856 -
Flags: review?(mats) → review+
Comment 52•7 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
Attachment #8936857 -
Flags: review?(mats) → review+
Comment 53•7 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
Attachment #8936858 -
Flags: review?(mats) → review+
Comment 54•7 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
Attachment #8936859 -
Flags: review?(mats) → review+
Comment 55•7 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
Attachment #8936860 -
Flags: review?(mats) → review+
Comment 56•7 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
Attachment #8936861 -
Flags: review?(mats) → review+
Comment 57•7 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
Comment 58•7 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
Attachment #8936862 -
Flags: review?(mats) → review+
Assignee | ||
Comment 59•7 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)
Assignee | ||
Comment 60•7 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.
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•7 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
Comment 69•7 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
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•7 years ago
|
status-firefox57:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•