131481 - Menulist with width constrained max-width and crop does not report correct size to enclosing box elements in dialogs

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Charles Manske]

I'll attach an example of Composer's publish dialog that has both 
style="max-width..." and crop attributes set. 
These do constrain the width of a menulist in the 2nd of 3 rows, but the grid
uses the preferred width instead of the constrained width. The result is that 
the grid width is too wide for the dialog and widgets get cutoff by the right
border of the dialog.

Comment 1

[Charles Manske]

Attachment: ZIP file with publish dialog to test bug

Unzip this attachment into mozilla/editor/ui/dialogs/content.
Make Composer to install into chrome.
Open Composer.
Use File | Publish
Go to the Settings panel if you aren't there already
Enter in a long string (about 40 chars) for the Site Name
Be sure there's some text in the "Publish URL" input field.
Click on the "Publish" tab.
The text you typed will become the selected item in the 'Site' menulist.
It's width is constrained to only 10em, but the dialog will resize and show
chopped off widgets and missing buttons.

Comment 2

[Charles Manske]

This is probably not the fault of grid code.
I rewrote the Publish Dialog UI to put the menulist in a simpe hbox and it had
the same effect: the calculated width was constrained by CSS max-width style,
but the dialog resized using what the menulist width would have been if it were
not constrained ("prefered" width).
After discussion with Hewitt the problem seems to be in menulist code.
Bryner has made recent changes in menulist code, eg., nsMenuPopupFrame.cpp

Comment 3

[Charles Manske]

Changed summary 

Comment 4

[Charles Manske]

*** Bug 132111 has been marked as a duplicate of this bug. ***

Comment 5

[Peter Trudelle]

Is this the only dialog in the product that has this problem?  Is there a cheap
workaround?  Couldn't we just constrain the site name length?

Comment 6

[Charles Manske]

I tried to do that and we had to constrain it so much that you couldn't see 
enough of the name to be useful. Brade opposed this strongely.
Surely it's a fundamental problem -- widget's must report proper size to their
parents!

Comment 7

[Peter Trudelle]

It may be a fundamental problem, but if it only occurs in this dialog, and the
site name is only used to populate the list, then we should consider making a
very safe trade between the current defect and having a shorter site name, which
would certainly not be an nsbeta1+ defect on its own.  We have many more bugs
than we are going to be able to fix for MachV, so I need to look for workarounds
where they exist.  Also, a more involved fix for this could easiliy cause
regressions that are themselves worse than this bug.  Other possible workarounds
might be to constrain the width of the site popup, allow the dialog to stretch,
 move or remove the New Site button, or some combination of these.  Being
limited to 20-30 characters to describe a site name doesn't seem like much of a
constraint, considering we couldn't publish at all until now.  cc Brade for input.

Comment 8

[Peter Trudelle]

Charlie, I just discussed this with Syd, and he also thought it might be more
appropriate to workaround it for now.  We should ensure that a bug remains open
for the real problem/fix.

Comment 9

[Samir Gehani]

Nav triage team: nsbeta1+, adt2 per Peter's/Syd's suggestion.

Comment 10

[Brian Ryner (not reading)]

I'm no longer seeing this bug on my own build or in a nightly (trunk)...
Charlie, can you confirm this?

Comment 11

[Charles Manske]

You probably can't see because we hacked around it by truncating all the strings
we put in the menulist. Not the correct solution :-)

Comment 12

[Brian Ryner (not reading)]

Ok, to remove the hack, would I change:

    var menuitem = AppendLabelAndValueToMenulist(gDialog.SiteList,
                        TruncateStringAtWordEnd(name, 30, true), name);

to

    var menuitem = AppendLabelAndValueToMenulist(gDialog.SiteList,
                         name, name);

?

Comment 13

[Brian Ryner (not reading)]

Attachment: patch

This should fix the problem.

Comment 14

[Charles Manske]

Comment on attachment 79528 [details] [diff] [review]
patch

Tested with case for Composer Publish dialog and it works great.

Comment 15

[Charles Manske]

Comment on attachment 79528 [details] [diff] [review]
patch

r=cmanske

Comment 16

[Joe Hewitt (gone)]

Comment on attachment 79528 [details] [diff] [review]
patch

sr=hewitt

Comment 17

[Brian Ryner (not reading)]

checked in on the trunk.  Requesting ADT approval for branch checkin.

Comment 18

[Charles Manske]

Attachment: Remove workarounds in Publish dialog

With above fix, we don't need any hacks in Composer. Yeah!

Comment 19

[Brian Ryner (not reading)]

Comment on attachment 79539 [details] [diff] [review]
Remove workarounds in Publish dialog

r=bryner

Comment 20

[jag (Peter Annema)]

Comment on attachment 79539 [details] [diff] [review]
Remove workarounds in Publish dialog

sr=jag

Comment 21

[Michael Buckland]

Please land and test on trunk.   Also please attach a screen shot of the problem
before the fix.

Comment 22

[Jaime Rodriguez, Jr.]

When's this gonna land on the trunk?

Comment 23

[Brian Ryner (not reading)]

It already has, that's why I marked it RESOLVED/FIXED.

Comment 24

[Syd Logan]

but, we need this screen shot for deciding if it goes in the branch. please add
one if possible, or update the bug if not so we can try some other way to
determine impact.

Comment 25

[Charles Manske]

Attachment: Screen shots for Publishing dialog

Top 2 images show dialog in correct form.
Next 2 show how we control the resizing by an ugly hack that truncates the 
sitename string.
Last image shows the serious basic problem: the menulist doesn't respect the 
"max-width: 30em" set in the dialog XUL and the result messes up dialog layout
such that buttons aren't useable.
The fix Bryner suggests in patch 79528 is very sound and will prevent similar 
problems by all menulist users. It has been working fine in the trunk -- I've 
found no bad side effects.
Bryner: Will this affect sizing of HTML select elements use in web page forms?
If yes, that would be a very strong argument for fixing this basic layout 
problem in the release branch.

Comment 26

[Syd Logan]

Good work, but let's save this for rtm, marking adt2 rtm, adt1.0.0-

Comment 27

[Brian Ryner (not reading)]

This would only affect HTML select sizing if XBL form controls were being used,
which they won't be for MachV or Mozilla 1.0.

Comment 28

[Charles Manske]

*** Bug 143789 has been marked as a duplicate of this bug. ***

Comment 29

[Charles Manske]

Getting dups on this already from beta build. This is important to fix for RTM.
Removed "-" so this will be evaluated for RTM checkin.

Comment 30

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on behalf ADT's behalf) for checkin to the 1.0 branch, pending
Driver's approval. Pls check this in asap.

Comment 31

[Syd Logan]

Adding the + in the keywords section (Jaime commented it was going to be plused
but change was not made)

Comment 32

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1"
keyword and add the "fixed1.0.1" keyword.

Comment 33

[Brian Ryner (not reading)]

checked into the 1.0 branch.

Comment 34

[shrirang khanzode]

verified101 on 0723 brnch build