Open Bug 1493962 Opened 11 months ago Updated 16 days 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, NeedInfo)

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

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=

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

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)
You need to log in before you can comment on or make changes to this bug.