1795049 - Right hand preference pane no longer flexes width based on content and available window space

Status: ASSIGNED

(Firefox :: Settings UI, defect, P1)

Description

[:Gijs (out for now; he/him)]

It's not really clear to me what determines the size of the main pane at this point. It used to be related to all its content, but it no longer seems to be. Changing the width of the browser window does not alter the position of the search box nor does it cause text in that pane to wrap more/less depending on the available amount of space.

This might be OK if everything genuinely fits like this, also in all other languages, but I'm concerned that the change in behaviour effectively masks one of:

1. text being cut off because the column is now too narrow for it
2. the column ending up way too wide on other locales where text is longer, and not shrinking as/when it should

It's not clear to me how bug 1793116 relates, and if that is a 107 nightly regression caused by bug 1790307 or if it is also broken on beta/release DE builds.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1795049 - Keep shrinking main pane with viewport width. r=Gijs

This addresses only part of it, but I think this should be
uncontroversial enough?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1795049 - Avoid text overlapping on the update radiogroup. r=Gijs

This can be easily tested shrinking the viewport width with the previous
patch applied and the accented locale. Text overlaps without this patch
when two adjacent radios wrap.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1790307

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1795049 - Improve text wrapping of XUL checkboxes. r=Gijs

Allow them to shrink under their content width (and thus wrap).

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0b4c4b02d82
Avoid text overlapping on the update radiogroup. r=Gijs,preferences-reviewers

Comment 7

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/b0b4c4b02d82

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1790307

Comment 9

[:Gijs (out for now; he/him)]

I feel responsible for getting this bug stuck, but we should probably figure out how to get it unstuck. Emilio, what do you think our options are at this point?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

So, https://phabricator.services.mozilla.com/D159277 I think should be fair and relatively uncontroversial, I think I just forgot to land it, should I do that?

Better wrapping for checkboxes and so would probably also be useful, though that's a bit more scary change. Maybe we should just put the learn more links inline in the markup. WDYT?

Comment 11

[:Gijs (out for now; he/him)]

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

So, https://phabricator.services.mozilla.com/D159277 I think should be fair and relatively uncontroversial, I think I just forgot to land it, should I do that?

Sounds good - can we still add the min-width: min-content or is there some reason that doesn't work?

Better wrapping for checkboxes and so would probably also be useful, though that's a bit more scary change. Maybe we should just put the learn more links inline in the markup. WDYT?

In an ideal world, I believe the following should be true:

• learn more links should be inline within any labels for checkboxes etc.
• checkbox labels should wrap when the available width requires it (rather than either horizontal scrollbars happening or the block width not shrinking any further)
• checkboxes should be aligned to the first line of wrapping labels, rather than vertically centered on the block of label text.

I suspect this will require a significant number of markup changes (and probably JS changes) to accomplish across the prefs. :-(

Unless I'm missing something?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5fcc08c508de
Keep shrinking main pane with viewport width. r=Gijs,settings-reviewers

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to :Gijs (he/him) from comment #11)

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

So, https://phabricator.services.mozilla.com/D159277 I think should be fair and relatively uncontroversial, I think I just forgot to land it, should I do that?

Sounds good - can we still add the min-width: min-content or is there some reason that doesn't work?

We could add it if you think the result is better, I just did.

In an ideal world, I believe the following should be true:

• learn more links should be inline within any labels for checkboxes etc.
• checkbox labels should wrap when the available width requires it (rather than either horizontal scrollbars happening or the block width not shrinking any further)
• checkboxes should be aligned to the first line of wrapping labels, rather than vertically centered on the block of label text.

I suspect this will require a significant number of markup changes (and probably JS changes) to accomplish across the prefs. :-(

Unless I'm missing something?

https://phabricator.services.mozilla.com/D161472 seems to go a long way without too invasive markup changes, wdyt? The key idea is to abuse the fact that in flexbox display: inline is blockified. So a <checkbox> with a wrapping <div> will be inline (and all the elements would wrap inside a single block formatting context, including the search more link etc), while a <checkbox> with a wrapping <hbox> / <vbox> would keep behaving as now for the most part. I'm not sure I'll have time to test that exhaustively and finish that up but maybe someone from the front-end team can?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: WIP: Bug 1795049 - Different approach to improve wrapping of checkboxes in preferences.

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5fcc08c508de

Comment 16

[:Gijs (out for now; he/him)]

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

https://phabricator.services.mozilla.com/D161472 seems to go a long way without too invasive markup changes, wdyt? The key idea is to abuse the fact that in flexbox display: inline is blockified. So a <checkbox> with a wrapping <div> will be inline (and all the elements would wrap inside a single block formatting context, including the search more link etc), while a <checkbox> with a wrapping <hbox> / <vbox> would keep behaving as now for the most part. I'm not sure I'll have time to test that exhaustively and finish that up but maybe someone from the front-end team can?

I'm seeing a few issues:

• the vertical alignment of the checkbox is out (too high) vs the label text
• the containers example now has the settings button wrap even on wide screens because of the flexing spacer
• the height of the changed labels is reduced, pulling the checkboxes closer together
• wrapped checkbox labels aren't indented (ie the second line of the label doesn't line up horizontally with the start of the first line)

I think I could probably work out how to fix the first few; I'm less sure about the last one.

It does also seem that the display: inline does cause alignment changes for checkboxes inside vbox and hbox, as well as groupbox (which I guess I assumed would be treated like hbox/vbox). Is that expected?

Comment 17

[Emilio Cobos Álvarez (:emilio)]

(In reply to :Gijs (he/him) from comment #16)

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

https://phabricator.services.mozilla.com/D161472 seems to go a long way without too invasive markup changes, wdyt? The key idea is to abuse the fact that in flexbox display: inline is blockified. So a <checkbox> with a wrapping <div> will be inline (and all the elements would wrap inside a single block formatting context, including the search more link etc), while a <checkbox> with a wrapping <hbox> / <vbox> would keep behaving as now for the most part. I'm not sure I'll have time to test that exhaustively and finish that up but maybe someone from the front-end team can?

I'm seeing a few issues:

• the vertical alignment of the checkbox is out (too high) vs the label text
• the containers example now has the settings button wrap even on wide screens because of the flexing spacer
• the height of the changed labels is reduced, pulling the checkboxes closer together
• wrapped checkbox labels aren't indented (ie the second line of the label doesn't line up horizontally with the start of the first line)

I think I could probably work out how to fix the first few; I'm less sure about the last one.

The only not totally gross way of doing that without changing the DOM structure I can think of would be something like adding a check-container class to the wrapping boxes and doing:

.check-container {
  margin-inline-start: 22px !important;
  text-indent: -22px;
}
.check-container.indent {
  margin-inline-start: 44px !important;
  text-indent: -22px;
}

Or so. Though at that point I'd probably try to rejigger the DOM (maybe not use <checkbox> at all and just use <input type="checkbox">, with a structure like:

<div>
  <input type="checkbox">
  <label for="checkbox">Stuff. <a class="learnMore">..</a>
</div>

Or so, because the negative text indent is kinda gross.

It does also seem that the display: inline does cause alignment changes for checkboxes inside vbox and hbox, as well as groupbox (which I guess I assumed would be treated like hbox/vbox). Is that expected?

Yeah, it's not totally unexpected. It's because the checkbox is display: block rather than display: -moz-box.

Comment 18

[Greg Hess]

Hi Emilio, is there anything further we can do on this issue.

Comment 19

[Greg Hess]

Hi Emilio, is there anything further we can do on this issue.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Not easily without doing more intrusive markup changes...

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:emilio, maybe it's time to close this bug?
For more information, please visit BugBot documentation.