Closed Bug 1784265 Opened 2 months ago Closed 1 month ago

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

Categories

(Core :: Layout, task)

task

Tracking

()

RESOLVED FIXED
105 Branch
Tracking Status
firefox105 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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

Attachment #9289458 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #9289458 - Flags: feedback?(bgrinstead)
Attachment #9289458 - Attachment description: WIP: Bug 1784265 - WIP: Drop support for flex attribute values other than 1. → WIP: Bug 1784265 - Drop support for flex attribute values other than 0 and 1.
Summary: Figure out what to do with flex="" attributes when emulating flexbox. → Figure out what to do with flex="" attributes when emulating -moz-box with flexbox.
See Also: → 1784408
Attachment #9289458 - Attachment description: WIP: Bug 1784265 - Drop support for flex attribute values other than 0 and 1. → Bug 1784265 - Drop support for flex attribute values other than 0 and 1. r=dholbert!,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

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

Depends on D154498

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.

Flags: needinfo?(mkmelin+mozilla)

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

Attachment #9289458 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
Pushed by ealvarez@mozilla.com:
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

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

Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 105 Branch
Regressions: 1784836

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

Flags: needinfo?(mkmelin+mozilla)
Regressions: 1789114
You need to log in before you can comment on or make changes to this bug.