combo boxes crop their longest string, in e.g. "Set as background image" dialog, "When Firefox Starts..." preferences menu, & about:permissions

VERIFIED FIXED in mozilla7

Status

()

Core
XP Toolkit/Widgets: Menus
--
trivial
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: pascalc, Assigned: dbaron)

Tracking

({regression})

Trunk
mozilla7
x86
Linux
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
Mozilla/5.0 (X11; Linux i686; rv:2.0b9pre) Gecko/20110107 Firefox/4.0b9pre

On Trunk, in the "Set as background image" dialog, the combox box with Stretch/Tile/Center options has cropped text on Linux. On XP I do not see this behaviour but the text is displayed in a smaller font size there compared to Clearlooks/Ambiance on Ubuntu.
(Reporter)

Comment 1

7 years ago
Created attachment 501996 [details]
Screenshot of the bug

Screenshot of the bug
(Reporter)

Comment 2

7 years ago
Created attachment 502001 [details] [diff] [review]
patch fixing the bug

I believe the menulist xul element should have the sizetopopup attribute set.

The patch I am attaching fixes the problem for me.
Assignee: nobody → pascalc
(Reporter)

Comment 3

7 years ago
Created attachment 502002 [details]
screenshot with the patch

Screenshot of the same dialog with the patch above applied
(Reporter)

Updated

7 years ago
Attachment #502001 - Flags: review?(dao)
(In reply to comment #0)
> On Trunk, in the "Set as background image" dialog, the combox box with
> Stretch/Tile/Center options has cropped text on Linux.

So is this a regression? When did it regress?
(Reporter)

Comment 5

7 years ago
I am not sure it is a regression since I don't use that option a lot and I only noticed it recently on en-US, not on the French version that I use for 3.6 and which has different string lengths.
(Reporter)

Comment 6

7 years ago
here is the result of a binary search with the mozregression tool

Last good nightly: 2010-09-15 First bad nightly: 2010-09-16

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0caec4ddff74&tochange=f38ef1080bfe
(In reply to comment #6)
> here is the result of a binary search with the mozregression tool
> 
> Last good nightly: 2010-09-15 First bad nightly: 2010-09-16
> 
> Pushlog:
> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0caec4ddff74&tochange=f38ef1080bfe

So my guess would be bug 566154.
Assignee: pascalc → nobody
Blocks: 566154
blocking2.0: --- → ?
Component: General → XP Toolkit/Widgets: Menus
Keywords: regression
Product: Firefox → Core
QA Contact: general → xptoolkit.menus
Version: unspecified → Trunk
Comment on attachment 502001 [details] [diff] [review]
patch fixing the bug

This seems to be a hack/workaround. The underlying regression should be fixed instead.
Attachment #502001 - Flags: review?(dao)
Minor visual annoyance in non-primary UI, so not going to hold the release for this bug. Blocking-.
blocking2.0: ? → -
I requested blocking because of a regression with unknown scope in core code.
Duplicate of this bug: 660187
Summary: in "Set as background image" dialog, the combox box with Stretch/Tile/Center options is cropped → combo boxes crop their longest string, in e.g. "Set as background image" dialog, "When Firefox Starts..." preferences menu, & about:permissions

Updated

6 years ago
Duplicate of this bug: 626159

Updated

6 years ago
Duplicate of this bug: 611991

Comment 14

6 years ago
** Disclaimer: I haven't actually looked very closely at the code yet, so this is just speculation based on some cursory observations; it could be wrong. **

I don't think this was caused by bug 566154's changes to the core, though it could (and probably should) be fixed in core.

The width of the menulist widget is the same as the width of the menulist menu popup, and that, in turn, is sized so that it is wide enough to accommodate both the text and the scrollbar (plus any necessary spacing).  Making the menulist menu popup wide enough appears to be what bug 566154 did.  The problem is that the menulist widget's minimum width needs to be at least equal to the width required by the text and the width taken up by the decorative dropmarker glyph on the right (plus any necessary spacing).

On Windows, the width of the menulist popup is an appropriate size for the menulist widget because the width of the scrollbar is about equal to the width of the dropmarker.  On Linux, the scrollbar is much thinner than that of other platforms, and the width taken by the dropmarker is much wider than that of other platforms, which I suspect is the cause of this problem.

Prior to bug 566154's landing, this was papered over by the large -moz-padding-end, but that padding was cut down (because it was no longer needed to support the menulist menu popup).

The quick fix would be to introduce extra padding, at least on Linux, and since that's a simple CSS-only fix for just gnomestripe, that should (hopefully) be low-risk enough to get into Aurora and maybe even Beta.

A more permanent fix that does calculations with the dropmarker width could be done for Firefox 7.

Comment 15

6 years ago
(In reply to comment #14)
Yes, my workaround is as follows.

menulist > menupopup > menuitem > label{
-moz-padding-start:3px !important;
-moz-padding-end:7px !important;
}
(Assignee)

Comment 16

6 years ago
This could also be a failure of some code to honor native-theme border.  With my theme (default on Ubuntu 11.04, I think), NS_THEME_DROPDOWN reports a right border of 22px to accommodate the button area.
(Assignee)

Comment 17

6 years ago
nsMenusFrame::SizeToContent appears to be dropping border and padding entirely (which its callers want included; see nsSprocketLayout::GetPrefSize); I think it should have an AddBorderAndPadding call.  What's not clear to me is whether that should be in addition to the scrollbar bit, as a max() with the scrollbar bit, or instead of the scrollbar bit (i.e., because the scrollbar thing was really a workaround for missing that).  Neil?
(Assignee)

Comment 18

6 years ago
Er, I should have said "nsMenuFrame::SizeToPopup".
(Assignee)

Comment 19

6 years ago
I tend to think it should be a max(), since we want a width such that:
 * the combobox and its popup are the same width
 * the content fits inside the popup (needs to allow scrollbar added)
 * the content fits inside the combobox part (needs to allow border/padding added)

I don't think there's any alignment requirement, so it seems like it's safe to add the max(scrollbarsizes.LeftRight(), borderPadding.LeftRight()).
(Assignee)

Comment 20

6 years ago
Created attachment 539378 [details] [diff] [review]
possible patch

Neil, does this match the intended use of nsMenuFrame::SizeToContent?  If you think it does, I'll try writing a test as well.
Attachment #539378 - Flags: review?(enndeakin)
After some debugging I think the right value to use is:

max(menulistPrefSize, popupSize + scrollbarWidth)

menulistPrefSize is the size of the menulist label + border, including any dropdown button area. The menulist should be at least this size. However, if the popup and scrollbars are larger, then the menulist should expand to fit this.
Created attachment 540542 [details] [diff] [review]
Something like this following
Attachment #540542 - Flags: review?
Attachment #540542 - Flags: review? → review?(dbaron)
(Assignee)

Comment 23

6 years ago
Given that, I'd presume we want to initialize tmpSize with 0 width rather than the current dummy -1 value.

But I don't understand why aSize would have anything to do with the amount of room needed for the contents... unless it's a function of the string that *currently* happens to be in the menulist, which seems like the wrong thing to depend on.  So does this really fix the bug, including for cases where a smaller item starts off in the menulist but then you switch to a larger one?
Comment on attachment 539378 [details] [diff] [review]
possible patch

OK, yes, that makes sense. When I tested your patch earlier, I saw much too wide menulists, but now I don't so I must have done something wrong then.
Attachment #539378 - Flags: review?(enndeakin) → review+
Attachment #540542 - Attachment is obsolete: true
Attachment #540542 - Flags: review?(dbaron)
Blocks: 667134
(Assignee)

Comment 25

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/79412e9e077a

I also wrote a test:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ab7641c5410f

Interestingly, the test fails on Windows both before and after the patch.  I wonder if there's a workaround for a related problem in the Windows theme...
Annotated menulist-shrinkwrap-2.xul as fails-if(Android), since so far it's looking like it consistently fails there too.
http://hg.mozilla.org/mozilla-central/rev/79412e9e077a
http://hg.mozilla.org/mozilla-central/rev/ab7641c5410f
and philor's annotation
http://hg.mozilla.org/mozilla-central/rev/eb42de70ee21
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7

Updated

6 years ago
Duplicate of this bug: 667134

Comment 29

6 years ago
Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110704 Firefox/7.0a1

Verified issue on Ubuntu 11.04, 10.10 x86 in the about:permissions menu.

Setting resolution to Verified Fixed.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

6 years ago
Assignee: nobody → dbaron
You need to log in before you can comment on or make changes to this bug.