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

RESOLVED FIXED in mozilla1.9.3a1

Status

()

defect
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: paulc, Assigned: iann_bugzilla)

Tracking

Trunk
mozilla1.9.3a1
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

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 :)
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.
Version: 2.0 Branch → unspecified
*"Restore Default Set"
Keywords: intl
Hardware: PowerPC → All
Summary: Default "Customize toolbars" window hides away the "Restore defaults" button → "Customize toolbars" window a bit too small on linux - cuts off the "Restore defaults" button
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).
Assignee: nobody → iann_bugzilla
Status: NEW → ASSIGNED
Attachment #406887 - Flags: review?
Attachment #406887 - Flags: review? → review?(gavin.sharp)
Component: Toolbars → Toolbars and Toolbar Customization
Product: Firefox → Toolkit
QA Contact: toolbars → toolbars
Version: unspecified → Trunk
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.
(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;
Attachment #406887 - Flags: review?(gavin.sharp)
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.
Attachment #406887 - Attachment is obsolete: true
Attachment #406918 - Flags: review?(gavin.sharp)
Posted patch SM/TB patch v0.1a (obsolete) — Splinter Review
This patch will fix SM/TB if the toolkit patch gets the go-ahead.
Attachment #406922 - Flags: review?(neil)
Would it be better to put the height and width on the panel instead?
Attachment #406918 - Flags: review?(gavin.sharp)
Screen shot of sheet under linux with Neil's onpopupshown patch.
Moved style from panel to iframe helps a bit under linux but still at 0,0
Attachment #409552 - Attachment is obsolete: true
Attachment #406922 - Flags: review?(neil)
Posted patch Use ch/em for mc patch v0.1b (obsolete) — Splinter Review
Changes since v0.1a
* As suggested by Neil, use onpopupshown event to reposition sheet.
Attachment #406918 - Attachment is obsolete: true
Attachment #412722 - Flags: review?(gavin.sharp)
Posted patch Use ch/em for cc patch v0.1b (obsolete) — Splinter Review
Changes since v0.1a:
* Use method suggested by Neil for repositioning the sheet.
Attachment #406922 - Attachment is obsolete: true
Attachment #412724 - Flags: superreview?(neil)
Attachment #412724 - Flags: review?(mnyromyr)
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)
(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.
Attachment #412722 - Flags: review?(gavin.sharp)
Posted patch Revised 86ch patch v0.1b1 (obsolete) — Splinter Review
Changes since v0.1b:
* Narrowed width of dialog to work better on OSX, still looks ok on Win/Linux.
Attachment #412722 - Attachment is obsolete: true
Attachment #413955 - Flags: review?(gavin.sharp)
Attachment #409554 - Attachment is obsolete: true
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.]
Attachment #412724 - Flags: superreview?(neil) → superreview+
Comment on attachment 412724 [details] [diff] [review]
Use ch/em for cc patch v0.1b

For mail/ review
Attachment #412724 - Flags: review?(bugzilla)
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 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 ;-)
Attachment #412724 - Flags: review?(bugzilla) → review?(philringnalda)
Attachment #413955 - Flags: review?(gavin.sharp) → review+
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.
Attachment #412724 - Flags: review?(mnyromyr) → review+
Attachment #412724 - Flags: review?(philringnalda)
Carrying forward r(SM) and sr.
Attachment #412724 - Attachment is obsolete: true
Attachment #421934 - Flags: superreview+
Attachment #421934 - Flags: review+
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.
Attachment #421934 - Flags: review?(philringnalda)
> -  <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 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.
Attachment #421934 - Flags: review?(philringnalda) → review+
Carrying forward r=
Attachment #413955 - Attachment is obsolete: true
Attachment #422832 - Flags: review+
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
Attachment #422832 - Attachment description: Unbitrotted revised 86ch patch v0.1b2 → Unbitrotted revised 86ch patch v0.1b2 [MC Checkin: Comment 27]
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
Attachment #421934 - Attachment description: Unbitrotted use ch/em for cc patch v0.1b2 → Unbitrotted use ch/em for cc patch v0.1b2 [CC Checkin: Comment 28]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Depends on: 549562
Depends on: 549445
Depends on: 566424
Depends on: 566425
You need to log in before you can comment on or make changes to this bug.