Last Comment Bug 623922 - combo boxes crop their longest string, in e.g. "Set as background image" dialog, "When Firefox Starts..." preferences menu, & about:permissions
: combo boxes crop their longest string, in e.g. "Set as background image" dial...
: regression
Product: Core
Classification: Components
Component: XP Toolkit/Widgets: Menus (show other bugs)
: Trunk
: x86 Linux
: -- trivial (vote)
: mozilla7
Assigned To: David Baron :dbaron: ⌚️UTC-10
: 611991 626159 660187 667134 (view as bug list)
Depends on:
Blocks: 566154 667134
  Show dependency treegraph
Reported: 2011-01-07 08:27 PST by Pascal Chevrel:pascalc
Modified: 2011-07-06 16:35 PDT (History)
16 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Screenshot of the bug (29.86 KB, image/png)
2011-01-07 08:27 PST, Pascal Chevrel:pascalc
no flags Details
patch fixing the bug (1.11 KB, patch)
2011-01-07 08:35 PST, Pascal Chevrel:pascalc
no flags Details | Diff | Splinter Review
screenshot with the patch (19.94 KB, image/png)
2011-01-07 08:38 PST, Pascal Chevrel:pascalc
no flags Details
possible patch (1.77 KB, patch)
2011-06-14 17:20 PDT, David Baron :dbaron: ⌚️UTC-10
enndeakin: review+
Details | Diff | Splinter Review
Something like this following (1.41 KB, patch)
2011-06-20 11:16 PDT, Neil Deakin
no flags Details | Diff | Splinter Review

Description Pascal Chevrel:pascalc 2011-01-07 08:27:16 PST
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.
Comment 1 Pascal Chevrel:pascalc 2011-01-07 08:27:50 PST
Created attachment 501996 [details]
Screenshot of the bug

Screenshot of the bug
Comment 2 Pascal Chevrel:pascalc 2011-01-07 08:35:32 PST
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.
Comment 3 Pascal Chevrel:pascalc 2011-01-07 08:38:12 PST
Created attachment 502002 [details]
screenshot with the patch

Screenshot of the same dialog with the patch above applied
Comment 4 Dão Gottwald [:dao] 2011-01-07 08:49:06 PST
(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?
Comment 5 Pascal Chevrel:pascalc 2011-01-07 09:00:10 PST
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.
Comment 6 Pascal Chevrel:pascalc 2011-01-07 10:22:21 PST
here is the result of a binary search with the mozregression tool

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

Comment 7 Dão Gottwald [:dao] 2011-01-07 10:37:26 PST
(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:

So my guess would be bug 566154.
Comment 8 Dão Gottwald [:dao] 2011-01-11 07:32:57 PST
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.
Comment 9 Dietrich Ayala (:dietrich) 2011-01-12 09:07:19 PST
Minor visual annoyance in non-primary UI, so not going to hold the release for this bug. Blocking-.
Comment 10 Dão Gottwald [:dao] 2011-01-12 10:13:51 PST
I requested blocking because of a regression with unknown scope in core code.
Comment 11 Daniel Holbert [:dholbert] 2011-06-01 14:10:58 PDT
*** Bug 660187 has been marked as a duplicate of this bug. ***
Comment 12 Kai Liu 2011-06-12 22:43:49 PDT
*** Bug 626159 has been marked as a duplicate of this bug. ***
Comment 13 Kai Liu 2011-06-12 22:54:00 PDT
*** Bug 611991 has been marked as a duplicate of this bug. ***
Comment 14 Kai Liu 2011-06-12 23:22:29 PDT
** 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 Alice0775 White 2011-06-13 00:18:40 PDT
(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;
Comment 16 David Baron :dbaron: ⌚️UTC-10 2011-06-14 17:04:01 PDT
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.
Comment 17 David Baron :dbaron: ⌚️UTC-10 2011-06-14 17:07:07 PDT
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?
Comment 18 David Baron :dbaron: ⌚️UTC-10 2011-06-14 17:07:56 PDT
Er, I should have said "nsMenuFrame::SizeToPopup".
Comment 19 David Baron :dbaron: ⌚️UTC-10 2011-06-14 17:11:42 PDT
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()).
Comment 20 David Baron :dbaron: ⌚️UTC-10 2011-06-14 17:20:22 PDT
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.
Comment 21 Neil Deakin 2011-06-20 11:13:49 PDT
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.
Comment 22 Neil Deakin 2011-06-20 11:16:52 PDT
Created attachment 540542 [details] [diff] [review]
Something like this following
Comment 23 David Baron :dbaron: ⌚️UTC-10 2011-06-20 20:59:28 PDT
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 24 Neil Deakin 2011-06-22 10:29:55 PDT
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.
Comment 25 David Baron :dbaron: ⌚️UTC-10 2011-06-29 16:50:56 PDT

I also wrote a test:

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...
Comment 26 Phil Ringnalda (:philor) 2011-06-29 22:44:10 PDT
Annotated menulist-shrinkwrap-2.xul as fails-if(Android), since so far it's looking like it consistently fails there too.
Comment 28 Jonathan Kamens 2011-06-30 09:52:42 PDT
*** Bug 667134 has been marked as a duplicate of this bug. ***
Comment 29 George Carstoiu 2011-07-04 06:59:37 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.