1493962 - Read the XUL flexbox attributes on XUL elements in CSS flexbox when layout.css.emulate-moz-box-with-flex is set

Status: RESOLVED WONTFIX

(Core :: Layout: Flexbox, enhancement, P3)

Description

[Brian Grinstead [:bgrins]]

This would fix functionality for [flex] and [ordinal] (as opposed to keeping a whitelist of known attr values), and probably improve performance.

If we could also include [width] and [height] I suspect that would fix some of the remaining functional breakage (like splitters).

Comment 1

[ExE Boss]

The easiest way to do this would probably be to use the CSS Values 3 typed attr(…) function, which is being implemented in bug 435426.

Comment 2

[Cameron McCormack (:heycam)]

That's true, but I don't think bug 435426 will be worked on soon.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Cameron McCormack (:heycam) from comment #2)
> That's true, but I don't think bug 435426 will be worked on soon.

Yeah, as nice as it'd be to use bug 435426, there's a simpler approach for this case. Neil has a patch from year or so ago that pretty much did this (with the exception of width/height), and I think it'll be able to be revived behind the emulation pref.

Comment 4

[Neil Deakin]

Attachment: This was the patch I had, last touched in September 2017

Comment 5

[Neil Deakin]

Attachment: Support attributes in flex

Comment 6

[Brian Grinstead [:bgrins]]

I went ahead and applied this locally, and did a few of try pushes (will take a while for results):

1. Talos push with pref flipped (baseline is without pref flipped): https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=03d8d0286112d5869f6c48f2ad26c627cb40a7b4&newProject=try&newRevision=bf30e2ca6295471b621977a7bf87efc8da05954e&framework=1&showOnlyImportant=1
2. Talos push with pref flipped (baseline is with pref also flipped but patch not applied): https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=&newProject=try&newRevision=bf30e2ca6295471b621977a7bf87efc8da05954e&framework=1&showOnlyImportant=1
3. Try push with -u all and pref flipped: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f14bdfb430f9a842a4c930dfc5b0d3facddd7315

Comment 7

[Brian Grinstead [:bgrins]]

And:
4. Try push with -u all and pref flipped (with this patch not applied): https://treeherder.mozilla.org/#/jobs?repo=try&revision=d24d72198dc3c3d25cc40c6c6c8b5ea23d13cb5a

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: current-mc-with-emulation.png

Uploading a screenshot before/after this patch

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: with-patch-emulation.png

A couple of issues I see with the patch applied:

1. The "Search with" text in url bar is centered
2. The content area is shorter than the window height

It's possible (2) is related to the special rules we have for <browser> https://searchfox.org/mozilla-central/rev/152993fa346c8fd9296e4cd6622234a664f53341/toolkit/content/xul.css#167 (I haven't checked). Here's all the special CSS rules we use when in emulation mode: https://searchfox.org/mozilla-central/search?q=moz-bool-pref%28%22layout.css.emulate-moz-box-with-flex%22%29&path=

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #9)

Created attachment 9041232 [details]
with-patch-emulation.png

A couple of issues I see with the patch applied:

1. The "Search with" text in url bar is centered

This can be fixed by increasing moz-box-flex at https://searchfox.org/mozilla-central/rev/ddd1679c0534f7ddf36cafddd17b710c4fefe3c4/toolkit/themes/osx/global/textbox.css#27, but this may be highlighting a bug with emulation or with the patch, since the sibling node (.urlbar-scheme) is an HTML input element with position:absolute.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 9040484 [details] [diff] [review]
Support attributes in flex

Review of attachment 9040484 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFlexContainerFrame.cpp
@@ +1321,5 @@
>    // --------------------------
>    float flexGrow, flexShrink;
>    if (IsLegacyBox(this)) {
> +    nscoord flex;
> +    nsFrame::AddXULFlex(aChildFrame, flex);

You don't want to make this call, if aChildFrame happens to be an anonymous flex item.  This is what's causing the issues that Brian mentioned in comment 9. (At least the first issue with the URLbar, and probably the second as well.)

If aChildFrame is an anonymous flex item, e.g. a wrapper for text or for an abspos placeholder, then we want it to just have default sizing properties -- but this change will make it potentially read flex information off of the associated DOM node (which I think is actually the flex *container*'s DOM node).

So: you probably want something like
 if (aChildFrame->Style()->IsAnonBox()) {
   flex = ChildFrame->StyleXUL()->mBoxFlex;
 } else {
   nsFrame::AddXULFlex(aChildFrame, flex);
 }

This is probably true for any other places in this patch where you read DOM attributes for a flex item.  If the flex item is anonymous, then its DOM node is the flex container itself, which isn't what you want to be reading flex-item attributes from.

Comment 12

[Neil Deakin]

Comment on attachment 9040484 [details] [diff] [review]
Support attributes in flex

The patch above is quite outdated, so please don't spend time looking at it right now. I'll be back next week and can provide a better version.

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Neil Deakin from comment #12)

Comment on attachment 9040484 [details] [diff] [review]
Support attributes in flex

The patch above is quite outdated, so please don't spend time looking at it
right now. I'll be back next week and can provide a better version.

Any progress here? I'd like to test out the new version when it's ready.

Comment 14

[Neil Deakin]

Attachment: cssflex2-rollup-apr18.txt

This is a rollup patch that contains many of the changes so far. Some fixes above the earlier patch:

1. Fixes the size of the browser content area and alignment in the url bar by not checking flex for anonymous items.
2. Fixes trees by using a minimum size instead of a fixed size.
3. Enables the emulated pref by default for easy testing.
4. Checks width/height for splitter and resizer. Splitter does not yet work as it expects it siblings to returns something useful for XUL layout methods. Resizer works ok.
5. Treats some emulated flexbox frames as being xul box frames as some callers expect this. Some of this isn't right but will be fixed up later.
6. Menupopups inherit from FlexContainerFrame and uses flexbox layout. Mostly this works but scrolling does not.

Comment 15

[Neil Deakin]

Some other pieces not included above as they are in various states of disarray and/or completion:

1. Getting splitter to work
2. Some hacks to get menus to scroll when a maximum height is set on the menupopup
3. Converting stacks/decks to use flexbox layout

Scrolling is a noteable issue. CSS generally only constrains the container (allowing scrolling) when a height is explicitly specified and just lets containers overflow when their contents are too large, whereas XUL layout will stretch flexible children to make them fit inside. For example:

<menupopup style="height: 100px">
<arrowscrollbox flex="1">
<scrollbox style="overflow: auto" flex="1">...</scrollbox>
<arrowscrollbox>
<menupopup>

In XUL layout, this creates a 100 pixel high menu with a scrollbox constrained to 100 pixels as the child is flexible. But in CSS layout, the scrollbox just extends outside its container.

This and related issues affects scrolling menus, as well as prevents scrollbars in the Preferences pane, and makes the page info window too large as the permissions pane won't shrink down.

Comment 16

[Neil Deakin]

Attachment: Bug 1493962, read the xul flex box attributes flex, orient, dir, align, ordinal and pack in nsFlexContainerFrame

Comment 17

[Neil Deakin]

Attachment: Bug 1493962, handle some calls to nsIFrame::IsXULBoxFrame as returning true when an emulated xul flex box

Depends on D31097

Comment 18

[Neil Deakin]

Attachment: Bug 1493962, when emulated flexbox is enabled, set width and height style instead of attributes, allowing resizer to work. This also handles the splitter, but that requires additional work as it relies on xul box methods to get children around it

Comment 19

[Neil Deakin]

The three patches here implement getting attributes. The two followup bugs fix some issues with trees and menus.

Comment 20

[Daniel Holbert [:dholbert]]

Neil: I am happy to address the review comments myself (requesting review from someone else wherever I'm changing something substantial) & shepherd this patch stack in, but there were several places where I didn't understand something about the changes.

Would you mind going through the phabricator feedback and replying to the non-trivial issues there, with any background/explanation that you can recall? I'm happy to take it from there.

Comment 21

[Daniel Holbert [:dholbert]]

Update: I'd been meaning to get to this and address my own review feedback, but haven't gotten to that yet (and I'm about to disappear for ~1 month for parental leave).

From conversations in slack & on other bugs with ntim and bgrins, I think (?) it's possible we might not actually need this patch stack anymore, too, but I might have misunderstood.

Anyway -- I'll plan on checking in with bgrins when I'm back from leave and seeing where things stand.

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Daniel Holbert [:dholbert] (blocking needinfo/review, due to AFK on parental leave 12/9 - 1/6) from comment #21)

Update: I'd been meaning to get to this and address my own review feedback, but haven't gotten to that yet (and I'm about to disappear for ~1 month for parental leave).

From conversations in slack & on other bugs with ntim and bgrins, I think (?) it's possible we might not actually need this patch stack anymore, too, but I might have misunderstood.

Anyway -- I'll plan on checking in with bgrins when I'm back from leave and seeing where things stand.

Yes, we are planning to stop relying on the attributes being read from layout and elsewhere as part of Bug 1596497 (which should handle both flexbox emulation and migration to HTML elements).

I suspect that there are parts of this stack or dependent bugs that are still important for emulated flexbox (like removing calls to nsIFrame::IsXULBoxFrame / GetChildXULBox and handling style.width/height on resizer), but I don't know exactly what is still relevant. Revisiting this when you are back sounds good, thanks.

Comment 23

[Emilio Cobos Álvarez [:emilio]]

We ported these to use regular style attributes etc.