1577257 - Site identity top panel height is not the same as the Connection Security top panel's

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P3)

Description

[Itiel]

Attachment: Site identity top panel height

See attached.

Changing this value here:
https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/browser/themes/shared/customizableui/panelUI.inc.css#1703
from 4px to 0.6rem fixes this for me, but I'm not sure where else that .panel-header may be in use which this change will not be suitable for.

Comment 1

[Itiel]

Attachment: Connection security top panel height

Comment 2

[Johann Hofmann [:johannh]]

Is this a recent regression? Paul and Nihanth have been working on this very recently, so that might have caused it

Comment 3

[Paul Zühlcke [:pbz]]

In Bug 1574196 the padding of the subview changed, Nihanth added some padding in Bug 1576929. Do we want it to have the same height? In this case we could consider using the built-in panelview header for the main view as well (now that the overflow is fixed).

Comment 4

[Johann Hofmann [:johannh]]

I would say that the same height (unless there's overflow) is preferable, yeah.

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

We have the same issue in the Protections Panel. I removed the height: 40px rule because it seemed unnecessary and didn't allow the header to grow for the overflow case, but it appears the mainview header has a min-height: 40px rule which is why it's bigger.

I think it should be sufficient to apply the min-height: 40px rule to the subview headers as well. Maybe even apply it to all PanelMultiView view headers.

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1577257 - Set a min-height on PanelMultiView headers for consistency. r=pbz

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

This bug is way more complicated than it looks at first glance, I don't think we can land this in 70.

Comment 8

[Itiel]

(In reply to Johann Hofmann [:johannh] from comment #2)

Is this a recent regression? Paul and Nihanth have been working on this very recently, so that might have caused it

This regressed by bug 1559258.

Comment 9

[Itiel]

Attachment: Bug 1577257 - Share logic behind panel headers across the UI r?gijs

Comment 10

[Itiel]

Hi emilio, I was told you could help me out here.

In the patch attached here I'm attempting to unify how the various panel headers are being created and displayed.
After making the needed changes, I got to a state in which the identity panel (when visiting https://permission.site/ for example) and the permissions panel are being cut off from the bottom. The hacky workaround I found to this was to add -moz-box-flex: 1 to them.
Somewhat related, the edit bookmark panel gets a permanent scrollbar no matter what (or how many) items I try to set display: none on.
All 3 panels get fixed if I either:

1. Replace the min-height of .panel-header in panelUI-shared.css with height (which is not a good fix because I want it to expand when the text inside is lengthy), or
2. Add overflow: hidden to .panel-header. This has no negative side effect AFAICS.

This raises a few questions though:

1. Is the overflow: hidden addition the right fix? If so, why? If not, what would you suggest as an alternative?
2. If I rename the permissions panel header to, say, 8 times of the following: Permissions for permission.site (with a whitespace between each repetition) and toggle overflow: hidden on and off, I can see a difference in the padding. Not a big deal but annoying. Anything that can be done about this? (ignore this question if overflow: hidden is not the solution)

Thanks!

Comment 11

[Itiel]

Attachment: Site identity being cut off

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Itiel from comment #10)

Hi emilio, I was told you could help me out here.

In the patch attached here I'm attempting to unify how the various panel headers are being created and displayed.
After making the needed changes, I got to a state in which the identity panel (when visiting https://permission.site/ for example) and the permissions panel are being cut off from the bottom. The hacky workaround I found to this was to add -moz-box-flex: 1 to them.

Yeah, so... You're mixing flexbox and XUL layout (like the <toolbarbutton>s etc) very indiscriminately, so I'm not surprised stuff gets a bit confused. In general it'd be great to move towards using HTML only in these panels, is that very hard? I see toolbarbutton doesn't honor display, but maybe it should? Or maybe we can turn them into regular html:buttons?

Somewhat related, the edit bookmark panel gets a permanent scrollbar no matter what (or how many) items I try to set display: none on.

Which panel is this? The star one, right?

All 3 panels get fixed if I either:

1. Replace the min-height of .panel-header in panelUI-shared.css with height (which is not a good fix because I want it to expand when the text inside is lengthy), or

Yeah, that's not good :/

2. Add overflow: hidden to .panel-header. This has no negative side effect AFAICS.

This adds a scroll frame between the flexbox and the xul box, so probably fixes some of the weirdness you're seeing (because XUL / HTML scroll frames are similar and a bit better tested). So if switching more fully to HTML layout is not feasible, and you don't want headers to overflow, this seems like a reasonable path forward.

This raises a few questions though:

1. Is the overflow: hidden addition the right fix? If so, why? If not, what would you suggest as an alternative?

It's the "right fix" in the sense that it sorta fixes it, because of the above. The real right fix is not mixing XUL and HTML layout modes to the extent possible.

2. If I rename the permissions panel header to, say, 8 times of the following: Permissions for permission.site (with a whitespace between each repetition) and toggle overflow: hidden on and off, I can see a difference in the padding. Not a big deal but annoying. Anything that can be done about this? (ignore this question if overflow: hidden is not the solution)

I didn't dig much into why, but I suspect a similar wrong interaction between XUL and HTML because I can't reproduce on a reduced HTML example...

Comment 13

[Itiel]

Attachment: Bug 1577257 - Share logic behind panel headers across the UI r?gijs!,willdurand,mak,dao

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d099556427a2
Share logic behind panel headers across the UI r=willdurand,Gijs,fluent-reviewers,flod

Comment 15

[Cosmin Sabou [:CosminS]]

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

Comment 16

[Itiel]

For the QE folks:
Please make sure that this patch hasn't regressed anything in panels in general, and that in ALL of them (across platforms) the panel header is the same height. Also test compact mode and touch mode.

Thanks!

Comment 17

[Ciprian Georgiu, Desktop QA]

I have reproduced this bug on an affected Nightly build 108.0a1 (20221103214316), with Win 10 x64.

The issue is verified as fixed following the instructions from the above comment, no issues were found while checking this on latest Beta 109.0ab9, across platforms: Win 10 x64, macOS 11 and Ubuntu 22.