Open Bug 1493962 Opened 2 years ago Updated 6 months ago

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

Categories

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

enhancement

Tracking

()

People

(Reporter: bgrins, Unassigned)

References

(Blocks 6 open bugs)

Details

Attachments

(5 files, 3 obsolete files)

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).
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.
Depends on: 435426
That's true, but I don't think bug 435426 will be worked on soon.
(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.
No longer depends on: 435426
Priority: -- → P3
See Also: → 435426
Attached patch Support attributes in flex (obsolete) — Splinter Review
Assignee: nobody → enndeakin
Attachment #9029700 - Attachment is obsolete: true

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

Uploading a screenshot before/after this patch

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=

Blocks: 1033225

(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 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 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.
Attachment #9040484 - Attachment is obsolete: true

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

Flags: needinfo?(enndeakin)
Attached patch cssflex2-rollup-apr18.txt (obsolete) — Splinter Review

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.
Flags: needinfo?(enndeakin)

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.

Blocks: 1551635
Blocks: 1551637

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

Blocks: 1558559
Blocks: 1434080
Attachment #9059323 - Attachment is obsolete: true
Assignee: enndeakin → nobody

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.

Flags: needinfo?(enndeakin)
See Also: → 1590576
See Also: → 1592369

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.

Flags: needinfo?(enndeakin)

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

You need to log in before you can comment on or make changes to this bug.