363858 - [FIX][reflow branch] Options in selects can have padding and margin now, which can result in exponential growth

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P4)

Description

[Martijn Wargers (dead)]

See upcoming testcase.
<option>'s (and <optgroup>'s) can now have margin and padding, which can result in really long comboboxes, like demonstrated in the testcase.
This changed between 2006-12-07 and 2006-12-08, so very likely something that changed with the landing of the reflow branch.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

I get some asserts too.  The issue is that IntrinsicForContainer returns an unconstrained pref width in the case when the percentages add up to 100% and a very large pref width when they just add up to close to 100%.

I'm really not quite sure what we can do about this, short of using !important to fix the margins/padding/borders of options inside a select.  Or somehow getting rid of the "min width is pref width" thing that <select> does.  I suppose we could get rid of it if we disabled all wrapping of options and just used the min-width of the block.

David, does anything obvious jump out at you here?

Oh, and a fieldset with a legend which has a div with percent padding/margins asa child has a similar problem, I bet...

Comment 3

[Daniel Holbert [:dholbert]]

See also bug 370872

Comment 5

[Daniel Holbert [:dholbert]]

Here are the reduced testcases from duplicate bug 370872:

attachment 256781 [details]
attachment 280133 [details]

Comment 6

[Daniel Holbert [:dholbert]]

One interesting thing I just discovered:  
  On Linux, the first time I click on a very-wide <select>, I get a dropdown menu.  BUT subsequent clicking on the <select> won't make it come back.  The only ways I can get it to come back are to either reload or to click a different drop-down menu.

e.g. on attachment 248667 [details]:
  Click the huge <select>. 
    --> wide dropdown appears.
  Click elsewhere to make it disappear.
  Click the big <select> again.
    --> ***nothing drops down***!!!   Weeeeeeird...
  Click a smaller <select>.
    --> smaller dropdown appears.
  Click elsewhere to make it disappear.
  Click big select again.
    --> wide dropdown appears again.  Yay!

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Can't return GetPrefWidth for GetMinWidth, since the former can be unconstrained...

Comment 8

[Boris Zbarsky [:bzbarsky]]

As a note, I'll fix fieldsets in bug 404123, I think.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 289275 [details] [diff] [review]
Proposed patch

r+sr=dbaron

Comment 10

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Missed a file in the patch

Luckily, the tests caught it.  ;)

Comment 12

[Ed Lee :Mardak]

Could this have regressed bug 406729?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Not likely, since this code isn't used by the url bar...

The scrolling fix in that date range is a more likely culprit imo.

Comment 14

[Martijn Wargers (dead)]

Verified fixed, using the testcase with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120405 Minefield/3.0b2pre

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 291167 [details] [diff] [review]
Missed a file in the patch

>Index: layout/forms/nsListControlFrame.cpp
> nscoord
> nsListControlFrame::GetMinWidth(nsIRenderingContext *aRenderingContext)
> {
>-  // We don't want to have options wrapping unless they absolutely
>-  // have to, so our min width is our pref width.
>+  // Scrollframes typically have an intrinsic min width of 0, but
>+  // that's not how we want to behave.

dholbert changed that today (?), but you may still want to override to add the scrollbar width

>   nscoord result;
>   DISPLAY_MIN_WIDTH(this, result);
> 
>-  result = GetPrefWidth(aRenderingContext);
>+  result = GetScrolledFrame()->GetMinWidth(aRenderingContext);
>+  nsBoxLayoutState bls(PresContext(), aRenderingContext);
>+  result += GetDesiredScrollbarSizes(&bls).LeftRight();

Are there ever cases where listboxes don't have scrollbars?  Should you check GetScrollbarStyles().mVertical ?  And what's the difference between GetScrollbarMetrics (which nsHTMLScrollFrame::GetPrefWidth uses) and GetDesiredScrollbarSizes?

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to comment #15)
> >+  // Scrollframes typically have an intrinsic min width of 0, but
> >+  // that's not how we want to behave.
> 
> dholbert changed that today (?)

Correct -- see bug 402567.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> but you may still want to override to add the scrollbar width

Yes, I do.  I saw that patch, and it adds the scrollbar width sometimes but not always.  Here we do want it always, since caller unconditionally subtracts it.

> Are there ever cases where listboxes don't have scrollbars?

No.  But there are times when a list control frame doesn't have a scrollbar: when it's the dropdown for a combobox with not too many items in it.  Nevertheless, we want to include the scrollbar with in the value in that case (since caller unconditionally subtracts it).

> And what's the difference 

None, if all you want is the pref size in the direction perpendicular to the scrollbar.  GetScrollbarMetrics is a static method that takes a pointer to a box and returns its GetMinSize/GetPrefSize (plus margin) as an nsSize.

GetDesiredScrollbarSizes takes the vertical and horizontal scrollbar boxes, gets the pref sizes (as nsSize), adds the margins, and returns the width of the vertical scrollbar and the height of the horizontal one, as well as information about which side the scrollbar is on (which my caller ignores).

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to comment #17)
> > but you may still want to override to add the scrollbar width
> 
> Yes, I do.  I saw that patch, and it adds the scrollbar width sometimes but not
> always.  Here we do want it always, since caller unconditionally subtracts it.

Actually, the final checked-in patch, "fix v3b", never adds scrollbar width.  It just returns the scrolled frame's min-width.

But it sounds like that probably doesn't affect you here, because you want to _always_ add scrollbar width.

For reference, "fix v3b" is at:
https://bugzilla.mozilla.org/attachment.cgi?id=291471&action=diff