1593303 - Stop using [flex] for xul flex styling and use a limited number of shared classes instead

Status: NEW

(Core :: XUL, task, P3)

Description

[Brian Grinstead [:bgrins]]

+++ This bug was initially created as a clone of Bug #1593023 +++

In order to support "vanilla" HTML box/hbox/vbox elements without Custom Elements, and to ease the transition for other HTML Custom Elements (bug 1563415), we should stop relying on [flex=N]. Instead we should identify the most common values and make classes (something like flex-1, flex-2, flex-10, flex-max) and then rewrite consumers (markup and JS) to use those. Consumers that don't match our specified flex values or ones that use ordinal should be rewritten in stylesheets / inline styles to use -moz-box-*.

Discussion in https://phabricator.services.mozilla.com/D51168#1561291.

Comment 1

[Brian Grinstead [:bgrins]]

Ordinal will be handled in Bug 1603830, so changing this to just [flex].

Comment 2

[Brian Grinstead [:bgrins]]

There aren't too many js consumers at least with simple string matching (~15 with most using a constant value assignment): https://searchfox.org/mozilla-central/search?q=.flex+%3D&case=true&regexp=false&path=.

There are a boatload of consumers in markup: https://searchfox.org/mozilla-central/search?q=flex%3D%22&case=true&regexp=false&path=.

I think it might make sense to go ahead and land something like this in xul.css which would allow us to incrementally convert frontend consumers.

*|*.flex-1 {
  -moz-box-flex: 1;
}

Technically i think it may need to be !important if we wanted to keep behavior identical with the layout code that reads [flex], but I think it's more clear going forward to do it without and hopefully(?) there aren't many races here with document css (typically you'd use either the attribute or css rules IME).

Mossop, what do you think about moving forward with that in relation to your firefox-dev post at https://groups.google.com/d/msg/firefox-dev/UuJISQiok38/I52n8tBlBgAJ?

Comment 3

[Dave Townsend [:mossop]]

Seems fine. Wouldn't just .flex-1 as the selector work?

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Dave Townsend [:mossop] (he/him) from comment #3)

Seems fine. Wouldn't just .flex-1 as the selector work?

If we add this to xul.css the default namespace is XUL so namespaceless selectors would only match XUL elements. Which would be fine right now but I'd like to also support HTML elements so we can migrate without frontend changes.

Comment 5

[Robin Steuber (she/her) [:bytesized]]

I've finally gotten a rough count of the values set for flex. This is with third-party and .gitignore-ed files omitted. I also did some simplistic filtering to try to weed out crash tests and anything that might be setting style.flex. But other than that, this is the raw output of my searching script so the counts may not be 100% accurate:

Flex:   Count
0:      4
1:      1158
2:      11
3:      7
4:      5
5:      3
7:      2
10:     4
15:     1
20:     2
25:     1
40:     1
80:     1
100:    8
175:    1
400:    1
1000:   3
6000:   1
10000:  1
60000:  1

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #5)

I've finally gotten a rough count of the values set for flex. This is with third-party and .gitignore-ed files omitted. I also did some simplistic filtering to try to weed out crash tests and anything that might be setting style.flex. But other than that, this is the raw output of my searching script so the counts may not be 100% accurate:

Flex:   Count
0:      4
1:      1158
2:      11
3:      7
4:      5
5:      3
7:      2
10:     4
15:     1
20:     2
25:     1
40:     1
80:     1
100:    8
175:    1
400:    1
1000:   3
6000:   1
10000:  1
60000:  1

Thanks for tracking that down. I wonder if we could treat anything over 10 or 100 as essentially the same and make a .flex-max class or something. It's technically different but practically it might not be visible.

In the meantime it's clear that 1 is the main target, so I do think it makes sense to add a .flex-1 or similar to xul.css and then start replacing the "easy" cases incrementally.