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

VERIFIED FIXED in mozilla1.9beta2

Status

()

Core
Layout: Form Controls
P4
normal
VERIFIED FIXED
11 years ago
5 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: bz)

Tracking

(Depends on: 1 bug, {regression, testcase})

Trunk
mozilla1.9beta2
x86
All
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [dbaron-1.9:RwCr])

Attachments

(3 attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 248667 [details]
testcase
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...
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
OS: Windows XP → All
See also bug 370872
Duplicate of this bug: 370872
Here are the reduced testcases from duplicate bug 370872:

attachment 256781 [details]
attachment 280133 [details]
Depends on: 393656
Blocks: 393656
No longer depends on: 393656
Whiteboard: [dbaron-1.9:RwCr]
Priority: -- → P4
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!
Blocks: 404281
Created attachment 289275 [details] [diff] [review]
Proposed patch

Can't return GetPrefWidth for GetMinWidth, since the former can be unconstrained...
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #289275 - Flags: superreview?(dbaron)
Attachment #289275 - Flags: review?(dbaron)
As a note, I'll fix fieldsets in bug 404123, I think.
Target Milestone: --- → mozilla1.9 M10
Summary: [reflow branch] Options in selects can have padding and margin now, which can result in exponential growth → [FIX][reflow branch] Options in selects can have padding and margin now, which can result in exponential growth
Comment on attachment 289275 [details] [diff] [review]
Proposed patch

r+sr=dbaron
Attachment #289275 - Flags: superreview?(dbaron)
Attachment #289275 - Flags: superreview+
Attachment #289275 - Flags: review?(dbaron)
Attachment #289275 - Flags: review+
Checked in.
Flags: in-testsuite+
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Created attachment 291167 [details] [diff] [review]
Missed a file in the patch

Luckily, the tests caught it.  ;)
Attachment #291167 - Flags: review?(dbaron)

Comment 12

10 years ago
Could this have regressed bug 406729?
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.
(Reporter)

Comment 14

10 years ago
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
Status: RESOLVED → VERIFIED
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?
Attachment #291167 - Flags: superreview+
Attachment #291167 - Flags: review?(dbaron)
Attachment #291167 - Flags: review+
(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.
> 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).
(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
Depends on: 422128
Depends on: 789759
You need to log in before you can comment on or make changes to this bug.