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

RESOLVED FIXED in mozilla12

Status

()

Toolkit
Toolbars and Toolbar Customization
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dao, Assigned: jaws)

Tracking

({regression})

Trunk
mozilla12
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 584965 [details]
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.
Yeah, I can take this.
Status: NEW → ASSIGNED
Created attachment 586783 [details] [diff] [review]
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.
Attachment #586783 - Flags: review?(dao)
(Reporter)

Comment 3

6 years ago
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?
Attachment #586783 - Flags: review?(dao) → review-

Comment 4

6 years ago
Jared, did you also test without aero; with Windows classic theme on Windows7; and with luna on XP?
Created 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. I don't have access to a WinXP machine.

Dao or Phillip: If you have WinXP, can you please test this patch?
Attachment #586783 - Attachment is obsolete: true
Attachment #587181 - Flags: review?(dao)
(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).
(Reporter)

Comment 7

6 years ago
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?
(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.
(Reporter)

Comment 9

6 years ago
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

6 years ago
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

6 years ago
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.
(Reporter)

Comment 12

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

Comment 13

6 years ago
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

6 years ago
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?
Created attachment 587593 [details] [diff] [review]
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?
Attachment #587181 - Attachment is obsolete: true
Attachment #587181 - Flags: review?(dao)
Attachment #587593 - Flags: feedback?(dao)
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
Created attachment 587612 [details] [diff] [review]
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.
Attachment #587612 - Flags: review?(gavin.sharp)

Comment 18

6 years ago
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

6 years ago
(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.
(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

6 years ago
> The buttons at the bottom were clipped before the change of styling.
Bug number?
(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.
(Reporter)

Comment 23

6 years ago
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.
Attachment #587612 - Flags: review?(gavin.sharp) → review-
(Reporter)

Comment 24

6 years ago
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.
Attachment #587593 - Flags: feedback?(dao) → feedback+
Created attachment 588713 [details] [diff] [review]
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?
Attachment #587593 - Attachment is obsolete: true
Attachment #587612 - Attachment is obsolete: true
Attachment #588713 - Flags: feedback?(dao)
Created attachment 589641 [details] [diff] [review]
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?
Attachment #588713 - Attachment is obsolete: true
Attachment #588713 - Flags: feedback?(dao)
Attachment #589641 - Flags: feedback?(dao)
Dao: feedback ping?
(Reporter)

Comment 28

6 years ago
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?
Created attachment 592232 [details] [diff] [review]
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.
Attachment #592232 - Flags: review?(dao)
(Reporter)

Updated

6 years ago
Attachment #592232 - Flags: review?(dao) → review+
Attachment #589641 - Flags: feedback?(dao)
https://hg.mozilla.org/mozilla-central/rev/c77a616744a2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla12
http://hg.mozilla.org/comm-central/rev/1694822c492e
Fix comm-central bustage from string rename and change in m-c bug 714277
I have filed bug 730832 as a follow-up bug to implement the dynamic width calculation.
You need to log in before you can comment on or make changes to this bug.