1120189 - [in-content preferences] application pane: content type list has no longer a fixed height

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Sören Hentzschel]

Attachment: screenshot

Since a new style for the content type list in the application pane has landed (see screenshot) the list of content types has no longer a fixed height, instead all content types are shown and the list no longer scrollable. I think either it's a regression and the list should be have a fixed height again or the search preferences should also show all search engines - there is a fixed height. I would prefer the first option because there are a lot of content types in the list…

Comment 1

[Albert Scheiner [:alberts]]

Just a note: Option one will affect bug 1027652.

Comment 2

[Alfred Kayser]

Or make the list fit the pane. See my themes such as Nautipolis for an example.
#mainPrefPane{-moz-box-flex:1} makes it flexible within given space, instead of overflowing...

Comment 3

[Richard Marti (:Paenglab)]

Attachment: Bug1120189.patch

(In reply to Albert Scheiner [:alberts] from comment #1)
> Just a note: Option one will affect bug 1027652.

Bug 1027652 should maybe be wontfix as the second scrollbar is not from the pane but from the list in the pane.

(In reply to Alfred Kayser from comment #2)
> Or make the list fit the pane. See my themes such as Nautipolis for an
> example.
> #mainPrefPane{-moz-box-flex:1} makes it flexible within given space, instead
> of overflowing...

This doesn't fix this issue here as the mainPrefPane expands to show the whole list.

This patch re-adds the 'height: 500px' which was, as I think, accidentally removed in Bug 1087618.

Comment 4

[Albert Scheiner [:alberts]]

(In reply to Richard Marti (:Paenglab) from comment #3)
> Created attachment 8547168 [details] [diff] [review]
> Bug1120189.patch
> 
> (In reply to Albert Scheiner [:alberts] from comment #1)
> > Just a note: Option one will affect bug 1027652.
> 
> Bug 1027652 should maybe be wontfix as the second scrollbar is not from the
> pane but from the list in the pane.
> 

I think that's just different wording chosen. From what I can see on the screenshot (attachment 8442836 [details]) Guilherme seems to refer to the scrollbar on the page and the scrollbar IN the pane. I see/saw these two scrollbars as well, even though they are less obvious on OS X due to scrollbars fading out. It might still be a wontfix, but that's not for me to say.

Comment 5

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

Comment on attachment 8547168 [details] [diff] [review]
Bug1120189.patch

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

I'm not sure we want to place a specified height on here. You should read up on bug 396121 and let me know what you think.

I could see us doing a max-height though. The constraints have changed a lot now that it is not in a secondary dialog.

Comment 6

[Richard Marti (:Paenglab)]

Attachment: Bug1120189.patch

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> 
> I could see us doing a max-height though. The constraints have changed a lot
> now that it is not in a secondary dialog.

The max-height helps only when not enough applications are in the list to not fix the height to 500px. When the window is still to small the handlersView doesn't shrink to fit the window. Also no change with additionally removing the flex: 1 from handlersView.

But I think it's better with the max-height to not fix the height when it's not needed.

Comment 7

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

Comment on attachment 8548362 [details] [diff] [review]
Bug1120189.patch

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

Thanks for the patch. After some deeper investigating, it looks like this is not the approach that we should take.

http://mxr.mozilla.org/mozilla-central/search?string=BrowserPreferences shows that if the preferences have "BrowserPreferences" as the document-element's ID then the #handlersView will get a height of 25em on Windows. We can make the rule apply regardless of the "animated" attribute since the in-content preferences are the same across all platforms. (We'll want to keep the old rules around until we are sure that we won't need the old preferences implementation anymore.)

However, this also uncovers a bug with Sync that is relying on the BrowserPreferences ID to be used. I filed bug 1123696 about this.

Comment 8

[Richard Marti (:Paenglab)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Comment on attachment 8548362 [details] [diff] [review]
> Bug1120189.patch
> 
> Review of attachment 8548362 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch. After some deeper investigating, it looks like this is
> not the approach that we should take.
> 
> http://mxr.mozilla.org/mozilla-central/search?string=BrowserPreferences
> shows that if the preferences have "BrowserPreferences" as the
> document-element's ID then the #handlersView will get a height of 25em on
> Windows. We can make the rule apply regardless of the "animated" attribute
> since the in-content preferences are the same across all platforms. (We'll
> want to keep the old rules around until we are sure that we won't need the
> old preferences implementation anymore.)
> 
> However, this also uncovers a bug with Sync that is relying on the
> BrowserPreferences ID to be used. I filed bug 1123696 about this.

I'm not sure what I should do now.

Only add #handlersView { min-height: 25em; } or using height: 25em or also add the ID BrowserPreferences on the in-content prefs? With adding the ID the selectors in preferences.css wouldn't work as there is no animated attribute in in-content prefs.

Comment 9

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

Comment on attachment 8548362 [details] [diff] [review]
Bug1120189.patch

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

I didn't notice that the binding was for a prefwindow and as such doesn't apply for the in-content preferences.

Sorry for the back and forth on this, but with some more playing around with it the behavior of 'height: 25em;' comes out the best. On a clean profile on my machine, I see 8 application handlers by default. If the list gets really long this will still cap the height of the list to 25em.

r=me if you change this to 'height: 25em;'

Comment 10

[Richard Marti (:Paenglab)]

Attachment: Bug1120189.patch

Changed to height: 25em;

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/4fafebcfc9df

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4fafebcfc9df

Comment 14

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using latest Nightly 39.0a1 (buildID: 20150317030206) and latest Aurora 38.0a2 (buildID: 20150316004007).