439134 - "Customize toolbars" window a bit too small on linux - cuts off the "Restore defaults" button

Status: RESOLVED FIXED

(Toolkit :: Toolbars and Toolbar Customization, defect)

Description

[Paul Craciunoiu [:paulc]]

The summary pretty much says it all.
Build id:
Mozilla/5.0 (X11; U; Linux i686; ro; rv:1.8.1.15) Gecko/20080612 Firefox/2.0.0.15

Steps:
1. Fresh install Firefox in Ro- (Romanian) locale.
2. Go to View->Toolbars->Customize...

Actual results:
In the Customize window that opens, you can't see the "Restore defaults" button (on the far down-right corner).

Expected results:
Window has default size (or formatting) so that you can see all of the buttons, otherwise you may never know they're there :)

Comment 1

[Hasham]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9) Gecko/2008052912 Firefox/3.0

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1a1pre) Gecko/2008063002 Minefield/3.1a1pre

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.1pre) Gecko/2008063004 GranParadiso/3.0.1pre

Confirmed on FF3 en-US. Looks to be a problem in FF2, FF3, 3.0.1pre and 3.1pre.

STR:

1) Fresh install Firefox (any locale)
2) Go to View => Toolbars => Customize

Screenshot coming.

Comment 2

[Hasham]

Attachment: "Default default..." button is cut off by default.

Comment 3

[Hasham]

*"Restore Default Set"

Comment 4

[Ian Neal]

Attachment: Change from px to ch/em for customize toolbar dialog patch v0.1

The issue is that the size of this dialog is currently measured in px which can cause issues with sizing between different platforms. This patch switches to using ch for width and em for height which should fix the issue (assuming I have worked out the correct values).

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So this patch also changes the values on other platforms as Linux. Have you tested it there too? If not it would be great if you could create a tryserver build (do you have access?) to test the patch on all platforms.

Comment 6

[Ian Neal]

(In reply to comment #5)
> So this patch also changes the values on other platforms as Linux. Have you
> tested it there too? If not it would be great if you could create a tryserver
> build (do you have access?) to test the patch on all platforms.

Depends which platform was used as a reference when the dialog was initially designed. The patch as it currently stands makes it big enough for linux but to match the current size of the dialog box on windows you have to set it to:
width: 106ch; height: 36em;
There will be an issue on OSX though as the logic in BrowserCustomizeToolbar / goCustomizeToolbar / CustomizeMailToolbar is expecting the width to be in px for working out where to position the sheet, so would mean changing:
var sheetWidth = sheetFrame.style.width.match(/([0-9]+)px/)[1];
to something like:
var sheetWidth = sheetFrame.style.width.match(/([0-9]+)ch/)[1] * 6;

Comment 7

[Ian Neal]

Attachment: Change units and make sheets work under OS X again patch v0.1a

Changes since v0.1:
* Width and height now match old width/height under windows.
* Match against ch instead of px in JS logic and do a crude conversion into px.

Comment 8

[Ian Neal]

Attachment: SM/TB patch v0.1a

This patch will fix SM/TB if the toolkit patch gets the go-ahead.

Comment 9

[[e-mail]]

Would it be better to put the height and width on the panel instead?

Comment 10

[Ian Neal]

Attachment: Sheet under linux with onpopupshown patch

Screen shot of sheet under linux with Neil's onpopupshown patch.

Comment 11

[Ian Neal]

Attachment: Sheet under linux 2 with onpopupshown patch

Moved style from panel to iframe helps a bit under linux but still at 0,0

Comment 12

[Ian Neal]

Attachment: Use ch/em for mc patch v0.1b

Changes since v0.1a
* As suggested by Neil, use onpopupshown event to reposition sheet.

Comment 13

[Ian Neal]

Attachment: Use ch/em for cc patch v0.1b

Changes since v0.1a:
* Use method suggested by Neil for repositioning the sheet.

Comment 14

[Stefan [:stefanh]]

Comment on attachment 412722 [details] [diff] [review]
Use ch/em for mc patch v0.1b

+<!ENTITY dialog.style             "width: 106ch; height: 36em;">

I just tried the patch, and I think the window becomes too wide - the width increases with about 100px (738px vs 635px before)

Comment 15

[Stefan [:stefanh]]

(In reply to comment #14)
> (From update of attachment 412722 [details] [diff] [review])
> +<!ENTITY dialog.style             "width: 106ch; height: 36em;">
> 
> I just tried the patch, and I think the window becomes too wide - the width
> increases with about 100px (738px vs 635px before)

Erm, forgot the most important: I tried it on Mac.

Comment 16

[Ian Neal]

Attachment: Revised 86ch patch v0.1b1

Changes since v0.1b:
* Narrowed width of dialog to work better on OSX, still looks ok on Win/Linux.

Comment 17

[[e-mail]]

Comment on attachment 412724 [details] [diff] [review]
Use ch/em for cc patch v0.1b

[You don't seem to have a reviewer for the mail/ portions of the patch.]

Comment 18

[Ian Neal]

Comment on attachment 412724 [details] [diff] [review]
Use ch/em for cc patch v0.1b

For mail/ review

Comment 19

[Karsten Düsterloh]

I'm completely unable to reproduce the actual problem this should fix in the first place - my customization dialog fits nicely under Kubuntu 9.04/Raleigh with 636px width.
With both patches applied, the dialog is 689px wide, which, very surprisingly ;-), still fits.
(Both measured after restart with XUL cache and localstore.rdf cleaned before.)

Hence I can only do a technical review...

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 412724 [details] [diff] [review]
Use ch/em for cc patch v0.1b

Sorry for the delay, big review queue, so passing to philor as he'll be better than me on this sort of patch anyway ;-)

Comment 21

[Karsten Düsterloh]

Comment on attachment 412724 [details] [diff] [review]
Use ch/em for cc patch v0.1b

r=me for the suite part on a pure codewise base.

Comment 22

[Ian Neal]

Attachment: Unbitrotted use ch/em for cc patch v0.1b2 [CC Checkin: Comment 28]

Carrying forward r(SM) and sr.

Comment 23

[Ian Neal]

Comment on attachment 421934 [details] [diff] [review]
Unbitrotted use ch/em for cc patch v0.1b2 [CC Checkin: Comment 28]

Requesting r= for TB side.

Comment 24

[Philip Chee]

> -  <panel id="customizeToolbarSheetPopup" noautohide="true">
> +  <panel id="customizeToolbarSheetPopup"

More bit rot for you. Firefox has moved their panels and popups inside their main popupset.

Comment 25

[Phil Ringnalda (:philor)]

Comment on attachment 421934 [details] [diff] [review]
Unbitrotted use ch/em for cc patch v0.1b2 [CC Checkin: Comment 28]

I can't claim any more knowledge of whether it actually fixes the problem than anyone else has so far, but, sure, let's see.

Comment 26

[Ian Neal]

Attachment: Unbitrotted revised 86ch patch v0.1b2 [MC Checkin: Comment 27]

Carrying forward r=

Comment 27

[Ian Neal]

Comment on attachment 422832 [details] [diff] [review]
Unbitrotted revised 86ch patch v0.1b2 [MC Checkin: Comment 27]

http://hg.mozilla.org/mozilla-central/rev/b27f05c8743b

Comment 28

[Ian Neal]

Comment on attachment 421934 [details] [diff] [review]
Unbitrotted use ch/em for cc patch v0.1b2 [CC Checkin: Comment 28]

http://hg.mozilla.org/comm-central/rev/0c5f67d0e49f