1784265 - Figure out what to do with flex="" attributes when emulating -moz-box with flexbox.

Status: RESOLVED FIXED

(Core :: Layout, task)

Description

[Emilio Cobos Álvarez (:emilio)]

That's the big thing of the layout emulation that doesn't quite work.

When we were doing it via pref, we had:

https://searchfox.org/mozilla-central/rev/f655bdf6b4bf01b42609750ab94fc37635397260/toolkit/content/xul.css#663

But that's neither perfect nor behavior-preserving. In XUL, the attribute overrides any CSS, which is not true for this.

We have a lot of usage of the flex="" attribute, but it is mostly flex="1":

$ rg 'flex="' | rg -v 'flex="1"' | cut -d : -f 1 | rg -v '/reftest|/crashtest|/tests' | uniq | wc -l
11

So I think a concrete proposal could look like:

• Make flex="1" work via an attribute selector.
• Make flex="<other-stuff>" not work, both when emulating and when using XUL layout (that is, remove support for looking up the flex attribute from XUL layout, so that we have the same behavior everywhere).

That should be relatively straight-forward / easy to audit... Does that sound reasonable?

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1784265 - Drop support for flex attribute values other than 0 and 1. r=dholbert!,mconley!

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9289458 [details]
Bug 1784265 - Drop support for flex attribute values other than 0 and 1. r=dholbert!,mconley!

Gijs / Brian, wdyt of something like this? This should make legacy layout and flexbox emulation behave the same wrt the flex attribute.

This gives me a working browser UI but I need to fix a couple other places (not many, see comment 0 + a couple JS places that do setAttribute("flex")). If this seems reasonable, I can finish it up and I can give a heads-up / help fix thunderbird / pinebuild as needed. The relevant asserts should ensure that if they're not hit we're not changing behavior.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1784265 - Remove XULElement.flex WebIDL API. r=mconley

It's basically an alias of setAttribute("flex", value), and it has no
remaining usage in the tree.

Since it's less useful now, let's remove the WebIDL API in favor of CSS.

Do this as a separate patch so that thunderbird / pine / etc can revert
this patch for diagnostics / to find UI with behavior changes.

Depends on D154394

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1784265 - Remove layout diagnostic code. r=dholbert

Do this separately so that Thunderbird / pine can revert this patch and
find affected elements.

Depends on D154498

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Magnus, you might want to run a debug build of Thunderbird through try with the first patch in this bug, to find broken layouts that grepping the tree with rg 'flex="' | rg -v 'flex="1"' doesn't find. In general for one-off usages it might be worth just replacing flex="N" with style="-moz-box-flex: N". For more complex usage it is probably worth moving it to CSS proper.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

For posterity, I considered to make this work more similarly to how html attributes behave (mapping the flex attribute to -moz-box-flex in style).

But the resulting priority is different (XUL was making the attribute override CSS, while attribute mapping would work the other way around). Let me know if you'd find that preferable tho, it's possible to figure out when it differs from the mapped attribute as well...

Comment 7

[:Gijs (he/him)]

Comment on attachment 9289458 [details]
Bug 1784265 - Drop support for flex attribute values other than 0 and 1. r=dholbert!,mconley!

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

Comment on attachment 9289458 [details]
Bug 1784265 - Drop support for flex attribute values other than 0 and 1. r=dholbert!,mconley!

Gijs / Brian, wdyt of something like this? This should make legacy layout and flexbox emulation behave the same wrt the flex attribute.

This gives me a working browser UI but I need to fix a couple other places (not many, see comment 0 + a couple JS places that do setAttribute("flex")). If this seems reasonable, I can finish it up and I can give a heads-up / help fix thunderbird / pinebuild as needed. The relevant asserts should ensure that if they're not hit we're not changing behavior.

This seems reasonable enough to me.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcb1a053fbe2
Drop support for flex attribute values other than 0 and 1. r=dholbert,mconley,preferences-reviewers
https://hg.mozilla.org/integration/autoland/rev/e27b21c54b1f
Remove XULElement.flex WebIDL API. r=mconley
https://hg.mozilla.org/integration/autoland/rev/415da4b53bdd
Remove layout diagnostic code. r=dholbert

Comment 9

[Marian-Vasile Laza]

Backed out 3 changesets (Bug 1784265) for causing reftest failures on flex-emulation-1.xhtml.
Backout link
Push with failures <--> R1
Failure Log

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79ce2b08a99e
Drop support for flex attribute values other than 0 and 1. r=dholbert,mconley,preferences-reviewers
https://hg.mozilla.org/integration/autoland/rev/c67009279787
Remove XULElement.flex WebIDL API. r=mconley
https://hg.mozilla.org/integration/autoland/rev/644d39c51480
Remove layout diagnostic code. r=dholbert

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/79ce2b08a99e
https://hg.mozilla.org/mozilla-central/rev/c67009279787
https://hg.mozilla.org/mozilla-central/rev/644d39c51480

Comment 12

[Magnus Melin [:mkmelin]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

Magnus, you might want to run a debug build of Thunderbird through try with the first patch in this bug, to find broken layouts.

Thanks, it found one case.