396121 - make Applications prefpane the right height on all primary platforms

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: makes prefpane the right height

The Applications prefpane's richlistbox sizes itself to 300px in height, which is fine for the Mac, where the preferences dialog resizes itself to the size of each prefpane.

But on Linux and Windows, the dialog has a fixed size and is resizable by the user.  On those platforms, the dialog should have a flexible height, so that it fills the height of the prefpane and shows the user as many entries as possible.

And in the process of fixing this bug, we should probably strip out the unnecessary vbox that surrounds all the prefpane content, since other prefpanes don't use such a vbox (the prefpane element itself is vertically oriented, as its XBL binding adds an inner vbox that surrounds the children of the element), and it doesn't seem to be necessary.

Here's a patch that fixes the problem.  I've tested this on Mac and Linux, and it works on both those platforms.  I'll test on Windows as well soon.

Comment 1

[Elmar Ludwig]

Whether the preferences dialog resizes itself depends on the setting of browser.preferences.animateFadeIn, which is true by default on Mac and false elsewhere.

Is setting browser.preferences.animateFadeIn to a non-default value allowed and supposed to work? If so, the sizing of #handlersView should probably depend on the pref setting instead of the platform.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

rstrong might have some thoughts on this.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: unhorked

Unhorked, but not requesting review, as rstrong and I discussed that we should key flex vs. specific height off the value of the animateFadeIn preference, as Elmar has suggested.  So rstrong is going to make sure there's an attribute identifying the status of that preference on the prefwindow, and then I'll submit an updated patch to condition flex vs. specific height on the status of the preference.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Requesting wanted-1.9 for this Applications prefpane polish fix.

Comment 5

[Robert Strong (they/them - no direct email)]

I've taken a look at this and have it mostly working. The only problem left is when animateFadeIn is true and the first prefpane opened is the applications prefpane. Then when switching to other prefpanes the height is incorrect.

Comment 6

[Robert Strong (they/them - no direct email)]

Attachment: patch rev3

Hey Myk, this works on Windows... could you give this patch a try to see if it works on Linux?

Comment 7

[Robert Strong (they/them - no direct email)]

btw: please check the following with browser.preferences.animateFadeIn set to false and set to true:
1) opening with the Applications prefpane being the last one used and then switch to all other panes.
2) opening with the Main prefpane being the last one used and then switch to all other panes.
3) opening with the Tabs prefpane being the last one used and then switch to all other panes.

Comment 8

[Robert Strong (they/them - no direct email)]

btw #2: I checked this with a resizable prefwindow and it worked as expected. It resized in both directions when animateFadeIn was false and it resized in width when animateFadeIn was true.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

I tried the latest patch on Linux, and it mostly works.  Everything works fine when animateFadeIn is true, although I didn't see resizing of width.

But when animateFadeIn is false, the first time I view the Applications prefpane after opening the Preferences dialog, the richlistbox starts off short and then jumps to the full height of the dialog a few moments later.  And the richlistbox has no scrollbar, so every item that doesn't fit into the space allotted for it gets cut off.

Both of these issues are present regardless of which prefpane was last used when I open the Preferences dialog.  All the other prefpanes appear fine.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #9)
> I tried the latest patch on Linux, and it mostly works.  Everything works fine
> when animateFadeIn is true, although I didn't see resizing of width.
> 
> But when animateFadeIn is false, the first time I view the Applications
> prefpane after opening the Preferences dialog, the richlistbox starts off short
> and then jumps to the full height of the dialog a few moments later.  And the
> richlistbox has no scrollbar, so every item that doesn't fit into the space
> allotted for it gets cut off.
> 
> Both of these issues are present regardless of which prefpane was last used
> when I open the Preferences dialog.  All the other prefpanes appear fine.

These are happening because the richlistbox doesn't have any flex, even though the prefpane around it does.  To fix it, we just need to add flex to the richlistbox, i.e. something like:

+#handlersView {
+  -moz-box-flex: 1;
+}
+
#BrowserPreferences[animated="true"] #handlersView {
  height: 300px !important;
}

Alternately, we could add |flex="1"| to the richlistbox element.

Comment 11

[Robert Strong (they/them - no direct email)]

Attachment: patch - rev4

Thanks... I removed that while working on preferences.xml and forgot to put it back. This should cover it... care to give it a try?

Comment 12

[Robert Strong (they/them - no direct email)]

btw: I suspect the problem with the width not resizing with the animateFadeIn true case on Linux is that I explicitly declared resizable when opening the prefpane to test it.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #12)
> btw: I suspect the problem with the width not resizing with the animateFadeIn
> true case on Linux is that I explicitly declared resizable when opening the
> prefpane to test it.

Yup, this works great!

Comment 14

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #13)
> (In reply to comment #12)
> > btw: I suspect the problem with the width not resizing with the animateFadeIn
> > true case on Linux is that I explicitly declared resizable when opening the
> > prefpane to test it.
> 
> Yup, this works great!

Err, this was meant to be a response to comment 11, not comment 12, i.e.:

> Thanks... I removed that while working on preferences.xml and forgot to put it
> back. This should cover it... care to give it a try?

Yup, this works great!

Comment 15

[Robert Strong (they/them - no direct email)]

Comment on attachment 281883 [details] [diff] [review]
patch - rev4

Hey Mike, this makes it so that prefpanes can specify flex as before without regressing the other fixes I've made recently to prefwindow / prefpane as well as makes it so the Applications prefpane is able to use the entire area available when animateFadeIn is false.

Comment 16

[Robert Strong (they/them - no direct email)]

> stealing

Comment 17

[Mike Connor [:mconnor]]

Comment on attachment 281883 [details] [diff] [review]
patch - rev4

>Index: browser/themes/winstripe/browser/preferences/preferences.css

>+/* Applications Pane */
>+#BrowserPreferences[animated="true"] #handlersView {
>+  height: 300px !important;
>+}

please specify in em

>Index: browser/themes/pinstripe/browser/preferences/preferences.css


>+/* ----- APPLICATIONS PREFPANE ----- */
>+#BrowserPreferences[animated="true"] #handlersView {
>+  height: 300px !important;
>+}

same here

r=me with that

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: As checked in with em and without !important

Comment 19

[Robert Strong (they/them - no direct email)]

Checked in to trunk

Checking in mozilla/browser/components/preferences/applications.xul;
/cvsroot/mozilla/browser/components/preferences/applications.xul,v  <--  applications.xul
new revision: 1.3; previous revision: 1.2
done
Checking in mozilla/browser/themes/winstripe/browser/preferences/preferences.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/preferences/preferences.css,v  <--  preferences.css
new revision: 1.21; previous revision: 1.20
done
Checking in mozilla/browser/themes/pinstripe/browser/preferences/preferences.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/preferences/preferences.css,v  <--  preferences.css
new revision: 1.22; previous revision: 1.21
done
Checking in mozilla/toolkit/content/widgets/preferences.xml;
/cvsroot/mozilla/toolkit/content/widgets/preferences.xml,v  <--  preferences.xml
new revision: 1.65; previous revision: 1.64
done

Comment 20

[Robert Strong (they/them - no direct email)]

myk, 25em was 298px on my system

Comment 21

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #20)
> myk, 25em was 298px on my system

FWIW, 300px was just a ballpark guesstimate.  If anyone thinks another number makes more sense, I'm all ears (for example, we could make the list as tall as the content in the Main prefpane).

Thanks for the fix!