1476236 - Give applications list a min-height

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Jorg K (CEST = GMT+2)]

So you can see when there is more than one.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1476236-applications-min-height.patch

We're switching to richlistboxes in TB and I noticed with in-content prefs, the app list only shows one entry. Confusing :-(

Please redirect review to whoever you deem fit, I picked the first one from the list.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8992583 [details] [diff] [review]
1476236-applications-min-height.patch

Review of attachment 8992583 [details] [diff] [review]:
-----------------------------------------------------------------

We set a min-height on the #appList through CSS. Can Thunderbird do the same? https://searchfox.org/mozilla-central/search?q=%23appList+%7B&path=

Comment 3

[Jorg K (CEST = GMT+2)]

Thanks for the comment. Yes, we can do it any way we want, for the moment we added this:
https://searchfox.org/comm-central/rev/ce038a0381f75865f5ed7cd1bdea357d5b61f8fa/mail/components/preferences/applicationManager.xul#48

The question is: Given that FF has a min-height of 212px
https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/browser/themes/windows/preferences/applications.css#67
why doesn't it actually work?

In FF 61 (release version), the box is 30px high and you see exactly one application entry which is rather confusing, see my comment #1.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

It looks like your applicationManager.xul is missing the reference to applications.css.

The Firefox applicationManager.xul has a reference to applications.css:
https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/browser/components/preferences/applicationManager.xul#7

The Thunderbird applicationManager.xul is missing this reference:
https://searchfox.org/comm-central/rev/ce038a0381f75865f5ed7cd1bdea357d5b61f8fa/mail/components/preferences/applicationManager.xul#6

Comment 5

[Jorg K (CEST = GMT+2)]

Hmm, is there a misunderstanding? It is NOT working in FF, as I said:

(In reply to Jorg K (GMT+2) from comment #3)
> In FF 61 (release version), the box is 30px high and you see exactly one
> application entry which is rather confusing, see my comment #1.

Would you like me to attach a screenshot?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

I did a regression hunt and found that bug 1435688 fixed this. See bug 1435688 comment 17 and bug 1435688 comment 18.

We should get this uplifted to beta. It's too late to get this fixed for Release now. I'll attach a patch now.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1476236 - Set a minimum height for the applications list in the Application Manager.

This patch uplifts a portion of bug 1435688 to 62-beta where we do not have a minimum height set.

Review commit: https://reviewboard.mozilla.org/r/257930/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257930/

Comment 8

[:Paolo Amadini]

Comment on attachment 8993102 [details]
Bug 1476236 - Set a minimum height for the applications list in the Application Manager.

https://reviewboard.mozilla.org/r/257930/#review265082

Thanks! Yeah, the list having only one row was a long-standing situation, probably this dialog isn't used often enough for this to be noticed before.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8993102 [details]
Bug 1476236 - Set a minimum height for the applications list in the Application Manager.

Approval Request Comment
[Feature/Bug causing the regression]: long standing regression that was fixed on 63 by bug 1435688
[User impact if declined]: the applications dialog will only show one row of applications
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no, but this is part of the patch from bug 1435688 that has been on Nightly for over three weeks
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: adds include for CSS file and sets a minimum height on the list (basically just CSS changes for a less used part of preferences)
[String changes made/needed]: none

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8993102 [details]
Bug 1476236 - Set a minimum height for the applications list in the Application Manager.

Minor CSS fix for an issue already fixed in nightly. 
Let's uplift for beta 11.

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/84cb4ee17544

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

This patch only needed to land on Beta so I'll close it now. From my understanding, Target Milestone is used to mark which Nightly cycle that the patch landed in, not necessarily which release/branch it landed on which is what the tracking status flags are for (firefox62: fixed, firefox63: unaffected).