714277 - Customization palette has only three columns in a new profile on Windows

Status: RESOLVED FIXED

(Toolkit :: Toolbars and Toolbar Customization, defect)

Description

[Dão Gottwald [:dao]]

Attachment: screenshot

Prior to bug 419231 we had four columns, which looks a lot more intentional.

Jared, can you take this? If this doesn't get fixed, it seems that your patch is a net loss at least on Windows, so we'd need to back it out.

Comment 1

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

Yeah, I can take this.

Comment 2

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

Attachment: Patch for bug 714277

I have tested this on Windows 7 with a clean profile. Four columns and three rows are visible with a vertical scrollbar for more rows.

I would like to only set the outerWidth feature, but when only setting outerWidth the dialog appears with height of 0. Therefore, I have also set the outerHeight feature.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 586783 [details] [diff] [review]
Patch for bug 714277

It's not clear where these numbers come from. Can you please document this?

Comment 4

[Philip Chee]

Jared, did you also test without aero; with Windows classic theme on Windows7; and with luna on XP?

Comment 5

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

Attachment: Patch for bug 714277 v2

The outerWidth value was calculated by finding the minimum width required by Win7/Aero and Win7/Classic that will allow for four columns of items. I don't have access to a WinXP machine.

Dao or Phillip: If you have WinXP, can you please test this patch?

Comment 6

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

(In reply to Jared Wein [:jwein and :jaws] from comment #5)
> Created attachment 587181 [details] [diff] [review]
> Patch for bug 714277 v2

I also tested this patch on Windows 7 Basic (aka without Aero).

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 587181 [details] [diff] [review]
Patch for bug 714277 v2

>+    // The outerWidth value was calculated by finding the minimum width
>+    // required by Win7/Aero and Win7/Classic that will allow for four
>+    // columns of items.

What does "calculated by finding" mean?

Comment 8

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

(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 587181 [details] [diff] [review]
> Patch for bug 714277 v2
> 
> >+    // The outerWidth value was calculated by finding the minimum width
> >+    // required by Win7/Aero and Win7/Classic that will allow for four
> >+    // columns of items.
> 
> What does "calculated by finding" mean?

I resized the window smaller by one pixel at a time to find the smallest size that would show all four columns.

Before doing that, I tried to figure out the width of the window by adding up the widths of the objects inside of it, but there is vertical scrollbar and extra window padding resulting in a number that didn't make much sense. I don't think it is worth our time to algorithmically find the smallest number, since we won't be able to put the algorithm in to the code.

Comment 9

[Dão Gottwald [:dao]]

Then the comment is lying, since you didn't calculate anything.

Why are you using outerWidth? Do width/innerWidth work?

Can the window resize itself to a dynamically determined minimum width?

Comment 10

[Philip Chee]

And third party theme developers are going to be very unhappy with hard coded values in chrome that can't be adjusted from a theme.

Comment 11

[KLB]

I agree with Philip, hard coding these values are going cause all sorts of havoc for some theme developers.  Remember not all themes use the icon sizes that the default themes do.  Some themes may use really big icons (e.g. for visually impaired) or really small icons.  Developer MUST be able to adjust these values to suit the needs of their theme.

Comment 12

[Dão Gottwald [:dao]]

Big items will get you fewer columns, small items will get you more columns. That's not necessarily a problem.

Comment 13

[KLB]

Another though I had was that when I looked at the screen capture, the size of the panel was smaller than was needed for the bottom buttons.  The width of the panel needs to be large enough to make sure these buttons fit comfortably.

Comment 14

[patrickjdempsey]

To add to what KLB is saying, there may also be localization issues here.  What happens if in some other language like German the buttons don't fit and some are completely pushed off of the edge?

Comment 15

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

Attachment: Patch for bug 714277 v3

This patch resizes the window dynamically to determine the width when it is opened. I have tested this patch on Windows 7 with Aero, Windows 7 without Aero, and Windows 7 Classic mode.

There are two questions with this approach though:
1) I'm not sure how to preserve the size of the resized window. With new profiles, the window's outerHeight is 38px, and the width is incorrect (the cause of this bug). How important is it to preserve the size of the window for the next time it is reopened? If it is necessary, do you have any tips? Maybe we should just use the calculated dimensions as the minimum size and only resize to them if the preserved size is smaller?

2) I didn't measure the height for the window since there are a lot more elements to measure and we may just be fine with setting the height of the window equal to the width of the window. What are your thoughts on this?

Comment 16

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

We could also just increase the width that is set here, but it wouldn't be computed dynamically then.

http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/customizeToolbar.dtd#2

Comment 17

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

Attachment: Alternate patch for bug 714277

This patch uses the same approach that was used in bug 439134 and is safe for other locales since they can and have been able to update their DTD file.

I have tested this with Windows 7 Aero, Windows 7 Basic, and Windows 7 Classic.

Flagging Gavin for review since he reviewed the patch for bug 439134.

Comment 18

[Axel Hecht [:Pike]]

From an l10n pov, we should be able to set a min-width to make sure the buttons fit. If we think that needs to be done again now that the dialog looks different, we should probably change the key of that entity.

As for the height, I recall that we used the golden ratio for width to height for some dialogs in the past.

Comment 19

[KLB]

(In reply to Jared Wein [:jwein and :jaws] from comment #15)
> There are two questions with this approach though:
> 1) I'm not sure how to preserve the size of the resized window. With new
> profiles, the window's outerHeight is 38px, and the width is incorrect (the
> cause of this bug). How important is it to preserve the size of the window
> for the next time it is reopened? If it is necessary, do you have any tips?
> Maybe we should just use the calculated dimensions as the minimum size and
> only resize to them if the preserved size is smaller?

For some users, it could be really important to preserve the size of resized windows. Some themes have buttons that are of a non-standard size and they would need extra space. Also a minimum size limitation is a bad idea, because a user may have a legitimate need to make the window smaller for whatever reason.

The only thing that should be set is the default window size and this size needs to take into account that the buttons can be larger on some themes/locales than others.  Maybe, whatever is used to calculate the default width of the window should automatically calculate how much space is needed for the buttons at the bottom.

Comment 20

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

(In reply to Axel Hecht [:Pike] from comment #18)
> From an l10n pov, we should be able to set a min-width to make sure the
> buttons fit. If we think that needs to be done again now that the dialog
> looks different, we should probably change the key of that entity.

The buttons at the bottom were clipped before the change of styling.

Comment 21

[Philip Chee]

> The buttons at the bottom were clipped before the change of styling.
Bug number?

Comment 22

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

(In reply to Philip Chee from comment #21)
> > The buttons at the bottom were clipped before the change of styling.
> Bug number?

Maybe bug 171454, but I don't think large fonts are required. I couldn't find any other bugs.

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 587612 [details] [diff] [review]
Alternate patch for bug 714277

We need to change the entity name (dialog.dimensions?), since localizers need to review this.

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 587593 [details] [diff] [review]
Patch for bug 714277 v3

>+  // Resize dialog to fit four columns of items by default.
>+  let toolbarPaletteItems = document.getElementsByTagName("toolbarpaletteitem");
>+  if (toolbarPaletteItems.length) {
>+    let itemWidth = parseInt(aWindow.getComputedStyle(toolbarPaletteItems[0], null).marginLeft) +
>+                    parseInt(aWindow.getComputedStyle(toolbarPaletteItems[0], null).paddingLeft) +
>+                    toolbarPaletteItems[0].boxObject.width +
>+                    parseInt(aWindow.getComputedStyle(toolbarPaletteItems[0], null).paddingRight) +
>+                    parseInt(aWindow.getComputedStyle(toolbarPaletteItems[0], null).marginRight);
>+    let windowPadding = 2 * document.getElementById("palette-box").boxObject.x;
>+    // Add 20 pixels for the scrollbar width and 2 pixels for the borders of the vbox.
>+    width = windowPadding + (4 * itemWidth) + 22;

The hardcoded 22 is fairly ugly. Also, you should only make the window large when it's not large enough. I think we could add a container within the scrolling container and resize the window by the difference between the new container's width and itemWidth * 4.

>+    // XXXjwein Using width as height here since I don't think we want to go
>+    // through the hassle of calculating the height of all the elements.
>+    let height = width;
>+    aWindow.resizeTo(width, height);

Just keep the initial height and use resizeBy?

I think we should take both patches.

Comment 25

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

Attachment: WIP of patch for bug 714277

I've added the inner box and the window *should* now resize by the difference of 4*itemWidth and the inner box when the inner box is not big enough to fit four columns. However, this does not have the effect that I hoped.

The inner box has a boxObject.width of 126 when the window is loaded. Each item is 130 pixels wide (110 width + 4px padding + 16px margins). When calling |window.resizeBy(394, 0)|, the window doesn't get wide enough to fit four columns (the inner box is 479px wide, 25 pixels too narrow), and the window height is stuck at 38 pixels tall.

Do you know why calling window.resizeBy with a deltaY of 0 would stop the window from opening at the preserved height and why extra width added to the window isn't enough?

Comment 26

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

Attachment: WIP of patch for bug 714277 v1.1

This patch works around the issue that I was seeing with window.resizeBy by using setTimeout. It's an ugly hack that causes the window to appear out of position for a split second and then move to the correct position and dimensions.

Dao: Am I doing something wrong here that is causing me to need this hack?

Comment 27

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

Dao: feedback ping?

Comment 28

[Dão Gottwald [:dao]]

Can you prepare a fixed up version of attachment 587612 [details] [diff] [review] so that we can get that landed while figuring out the other patch?

Comment 29

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

Attachment: Alternate patch for bug 714277 (simpler fix)

(In reply to Dão Gottwald [:dao] from comment #28)
> Can you prepare a fixed up version of attachment 587612 [details] [diff] [review]
> [review] so that we can get that landed while figuring out the other patch?

Sure can! I've attached a fixed up version here.

Comment 30

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

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

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 32

[Serge Gautherie (:sgautherie)]

http://hg.mozilla.org/comm-central/rev/1694822c492e
Fix comm-central bustage from string rename and change in m-c bug 714277

Comment 33

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

I have filed bug 730832 as a follow-up bug to implement the dynamic width calculation.