314939 - Combobox dropdown menu too high

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Chris Hester]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051025 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051025 Firefox/1.5

When 20 options in a form are given a large enough height, Firefox displays the menu list so it goes off the screen.

Reproducible: Always

Steps to Reproduce:
1. style options tag to large height, eg: 90 pixels
2. add exactly 20 options in the form
3. click on select drop-down menu

Actual Results:  
Clicking on the menu produces a list that goes upwards and off the screen.

Expected Results:  
Now if 19 or less options are used in the form, Firefox will show them going downwards to fit the screen. If 21 options are used, it will automatically insert a scrollbar. But 20 options causes the bug.

The bug may not occur if the height of the options is small enough to fit the menu list on screen, or the screen has a large enough height. This can depend on the size of the Windows taskbar as well.

Comment 1

[Chris Hester]

Attachment: Basic form that demonstrates the bug

Note: comment out the 21st option in the form to see how a scrollbar is produced. Also play around with the height setting.

Comment 2

[Peter van der Woude [:Peter6]]

Attachment: testcase for 1024 px high screen

compare the 2 selectboxes

Comment 3

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051103 Firefox/1.5 ID:2005110310

confirmed

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Comment 5

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

We need to do the half screen size check also after limiting
the height to 20 rows. When this occurs try to at least show
two options, since it generally looks better then just one IMO.

(To test the border+padding adjustment you can for example increase
the border size in the rule with the "*|*::-moz-dropdown-list"
selector in the file "./dist/bin/res/forms.css")

Comment 6

[Mats Palmgren (inactive)]

Attachment: FYI, wip (diff -w)

Here's another case: when the vertical offset of the dropdown
is on the upper screen half AND it overflows the bottom screen edge
we display it above the combobox - even though there actually is
less space above. I made a patch for this that keeps it displayed
below because at first I thought this was an improvement.
But I have changed my mind.  First, with the first patch this should
occur very rarely, for really degenerated cases, like a single
>800px option for example. I think we should keep it displayed above
because it signals to the user that the menu was going to be
clipped if displayed below (the invariant being that "if below,
it's not clipped"). Anyway, this will never occur for normal pages
with the first patch.

Comment 7

[Boris Zbarsky [:bzbarsky]]

A few comments before I look at the patch:

1)  There's a similar bug already filed (by dougt, I believe).  Can't find it
    right now.
2)  GetScreenHeight is not exactly cheap.  Changing this would need some perf
    tests on all three platforms to see what the impact is (say on a large
    slashdot page with all the moderation comboboxes).
3)  All this code is totally different on the reflow branch, though with
    compatible final behavior.  I'd really like to not make changes to it on
    trunk until the branch lands.  Rewriting it once was bad enough; I'd really
    rather not do it multiple times...

Comment 8

[Martijn Wargers (dead)]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

To clarify item 3 of comment 7, unless we absolutely need to fix this ASAP for some reason I'd prefer we just fix it on the reflow branch instead.

Comment 10

[Mats Palmgren (inactive)]

I ran two tests on each of Windows XPSP2, Linux (gtk2) and MacOSX.
Testcase #1:  1000 <select>s with 20 <option>s each
Testcase #2:  3417 <select>s with 1 <option> each

I compared Reflow CPU time, avg. of five runs.
(- means faster with patch, + means slower with patch)
        Testcase #1      Testcase #2
WinXP      -6.8%            +0
Linux      +6.0%            +5.8
Mac        +0.1%            +1.2

The tests were run on trunk code, but the reflow-branch code looks
identical to me:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/forms/nsListControlFrame.cpp&rev=1.408&root=/cvsroot&mark=886-922#886
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/forms/nsListControlFrame.cpp&rev=REFLOW_20060830_BRANCH&root=/cvsroot&mark=715-749#715

Comment 11

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (same as rev.1 but for reflow-branch)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 242285 [details] [diff] [review]
Patch rev. 2 (same as rev.1 but for reflow-branch)

>Index: layout/forms/nsListControlFrame.cpp

>+  // We calculate the available space as half the screen height subtracted
>+  // with an estimated height of the combobox

s/subtracted with/minus/

>+  if (NS_SUCCEEDED(rv)) {

And if not, we end up using NS_UNCONSTRAINEDSIZE instead of clamping to kMaxDropDownRows?  That seems wrong.

This diff could _really_ use a -w, I think.  :(

>+    float p2t = aPresContext->PixelsToTwips();
>+        nscoord screenHeight = NSIntPixelsToTwips(screenHeightInPixels, p2t);

Weird indent.

>+    if (screenHeight/2 - heightOfARow*2 < visibleHeight) {
>+      visibleHeight = screenHeight/2 - heightOfARow*2;

I'd keep the local we used to have for |screenHeight/2 - heightOfARow*2|.  Probably faster, and definitely more self-documenting if it has a good name.

I'd like to see some comments explaining the sizing before the code dives into it.  For example, whence all the PR_MINs?  They didn't use to be there.  That is, what sizing are you trying to accomplish?  Why does it matter how borderpadding compares to the height of a row?

One last question.  As far as Linux goes, was that local X, or remote X?  What does remote X look like perf-wise?

Comment 13

[Mats Palmgren (inactive)]

The Linux figures above were remote X over SSH.
adding a local X run (still Patch 1/trunk) the table looks like this:

Reflow CPU time, avg. of five runs.
(- means faster with patch, + means slower with patch)
                       Testcase #1      Testcase #2
WinXP                     -6.8%            +0
Mac                       +0.1%            +1.2%
Linux-gtk2-remote-X       +6.0%            +5.8%
Linux-gtk2-local-X        +7.8%            +8.2%

Just to double check, I re-ran the baseline for the Linux tests
a second time and relative to this run the numbers above would be
about half, so I would say that the margin of error (in my Linux tests)
is a few percent.

Comment 14

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

I tried to address you concerns by simplifying the code a bit and adding
more comments. Hopefully it's clearer now. I also discovered a possible
optimization which improves performance quite a bit.

What we currently do is:
1. reflow once
2. clamp to 20 rows, then clamp again to ~screenheight/2
3. reflow a second time

I think we can skip 3 if 2 does not actually change the height,
which would be true for most <select>s with less than 20 options.
With this optimization, we can also avoid the expensive
  "visibleHeight = (visibleHeight / heightOfARow) * heightOfARow;"
in most cases since the new values we assign visibleHeight are
multiples of heightOfARow already.

Same testcases as above (this time on the reflow-branch), Patch 3:
Reflow CPU time, avg. of five runs.
(- means faster with patch, + means slower with patch)
                       Testcase #1      Testcase #2
Linux-gtk2-remote-X       -32%             -18%

A bit more analysis of ReflowAsDropdown() for "typical usage" -
Loading a Bugzilla bug page, zooming in/out, reloading etc:
408 calls to ReflowAsDropdown():
  0 first cut
170 second cut
182 third cut
The remaining calls (14%) does a 2nd reflow (down from 58%)

Loading hundreds of "top100 pages" (according to Alexa and other sources):
267 calls to ReflowAsDropdown():
  0 first cut
 92 second cut
100 third cut
The remaining calls (28%) does a 2nd reflow (down from 66%)

By first/second cut I mean these two early returns:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/forms/nsListControlFrame.cpp&rev=1.406.2.1&root=/cvsroot&mark=671,691-692#667

Third cut is the new early return(s) at the end of Patch 3.

Comment 15

[Mats Palmgren (inactive)]

A few more comments/questions:

Why do we assign "mNumDisplayRows = kMaxDropDownRows" here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/forms/nsListControlFrame.cpp&rev=1.406.2.1&root=/cvsroot&mark=716#683
This means for example that a <select> with less than 20 options will
still require 20 line scrolls to get to the bottom.
Is this intentional? 
(Wouldn't it make more sense to have mNumDisplayRows=MIN(actual rows,20))

Regarding the test for "heightOfARow > 0" - maybe we could test for zero
before returning from CalcFallbackRowHeight() and set it to 16px or whatever
so we don't need to worry about divide-by-zero elsewhere?
Something is really out-of-whack if CalcFallbackRowHeight() returns zero, no?
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/forms/nsListControlFrame.cpp&rev=1.406.2.1&root=/cvsroot&mark=1939#1904

Does this comment still make sense (can I help fixing it while I'm here?)
    // XXXbz this is ending up too big!!  Figure out why.

I'm not so sure "availDropHeight = screenHeight/2 - heightOfARow*2" is a good
heuristic. Zooming a dropdown menu to ~1000% makes it two rows high
(1600x1200 screen) when it could easily fit 4 or 5 rows on half the
screen height. "screenHeight/2 - heightOfARow/2" or even
"screenHeight/2 - 30px" or something might be better in general...

Comment 16

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3 (diff -w)

Comment 17

[Boris Zbarsky [:bzbarsky]]

> 3. reflow a second time

Hmm...  I guess since we're not basing our computed height on the height of a row, changing the height of a row doesn't need to lead to an automatic second reflow....  Good catch on that.

I'll have to think long and hard about the optimization changes.  Unfortunately, I'm not sure when I'll get the chance to do that.  Certainly not till sometime next week at the absolute best.  :(  I assume you've tested this on the bugs cited in the reflow branch checkins to this file?

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Why do we assign "mNumDisplayRows = kMaxDropDownRows" here:

It was easy, and gave the right general results when I tested.  I suppose we could do the PR_MIN you suggest, but it would give the same behavior, no?

> maybe we could test for zero before returning from CalcFallbackRowHeight()
> and set it to 16px or whatever

Yeah, that would work.  Then document accordingly and propagate out the not-checking... Sure.

>    // XXXbz this is ending up too big!!  Figure out why.

I don't actually know.  I can't recall which testcase was showing the problem.... :(

> I'm not so sure "availDropHeight = screenHeight/2 - heightOfARow*2" is a good
> heuristic.

It's not.  The right heuristic is height of a row plus the parent reflow state's computed border+padding-top/bottom, I'd think....  or maybe even factor in the parent reflow state's computed height?

Comment 19

[Mats Palmgren (inactive)]

(In reply to comment #17)
> I assume you've tested this
> on the bugs cited in the reflow branch checkins to this file?

I have to admit I didn't... and it does regress bug 349328 :-(
(bug 349250 seems fine though. These were the only two I could find
mentioned in the CVS log for this branch)

What happens with patch 3 for the testcase in bug 349328 is:

First call:
We do the first unconstrained reflow
result is visibleHeight=0
we force "visibleHeight = heightOfARow" (255)
we save mLastDropdownComputedHeight = 255
we do the second reflow at the end (with state.mComputedHeight = 255)

Second call:
We set state.mComputedHeight = 255 (from mLastDropdownComputedHeight)
(GetScrolledFrame()->GetSize().height is still zero)
We reflow
result is visibleHeight=1020
this does not need to be clamped, so we do my new optimisation and return...

Comment 20

[Boris Zbarsky [:bzbarsky]]

> These were the only two I could find mentioned in the CVS log for this branch

That sounds about right, yeah...

Comment 21

[Mats Palmgren (inactive)]

(In reply to comment #18)
> > Why do we assign "mNumDisplayRows = kMaxDropDownRows" here:
> 
> It was easy, and gave the right general results when I tested.  I suppose we
> could do the PR_MIN you suggest, but it would give the same behavior, no?

I see now that the current code actually do adjust mNumDisplayRows if the
height was clamped by screenheight/2... Also, mNumDisplayRows is only
used when scrolling with KEY_PAGE_UP/DOWN, not when clicking on the
scrollbar arrow buttons as I first thought.
I'm now updating mNumDisplayRows accordingly when we change the height.

> > maybe we could test for zero before returning from CalcFallbackRowHeight()
> > and set it to 16px or whatever
> 
> Yeah, that would work.  Then document accordingly and propagate out the
> not-checking... Sure.

Ok, did that.

> > I'm not so sure "availDropHeight = screenHeight/2 - heightOfARow*2" is a
> > good heuristic.
> 
> It's not.

I'm leaving it as is for now... I filed bug 357552 on it.

Comment 22

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4

I figured out what was wrong with the last patch. When the first reflow
is constrained we can still get a scroll frame height that differs but it will
be clipped as overflow. It only means that the saved height is obsolete.
We need to do the second reflow to make it size to the new actual height.
This is unusual however, in most cases it's either the same height (which
will be pruned by the "cut 2" early return) otherwise it's most likely
an unconstrained reflow and this case can still be optimised if the height
isn't clamped ("cut 3").

Same testcases as above (this time on the reflow-branch), Patch 4:
Reflow CPU time, avg. of five runs.
(- means faster with patch, + means slower with patch)
                       Testcase #1      Testcase #2
Linux-gtk2-remote-X       -29%             -30%


The distribution of the early returns is almost the same as with Patch 3:
Loading a Bugzilla bug page, zooming in/out, reloading etc:
663 calls to ReflowAsDropdown():
  0 first cut
289 second cut
280 third cut
The remaining calls (14%) does a 2nd reflow (down from 56%)

Loading hundreds of "top100 pages" (according to Alexa and other sources):
304 calls to ReflowAsDropdown():
  0 first cut
116 second cut
103 third cut
The remaining calls (28%) does a 2nd reflow (down from 62%)

This time I have tested more thoroughly, it does not regress bug 349250 or
bug 349328, and I tested *a lot* of other combobox bugs, testcases...

BTW, the "+#ifdef NS_DEBUG" hunk is to fix a compile warning in non-debug
builds (variable 'rv' not used).

Comment 23

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4 (diff -w)

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 243061 [details] [diff] [review]
Patch rev. 4

OK, so I think I buy the "if the height we compute is the height we already ended up with, no need to reflow" thing.  The thing is, we need to decide that in the nsSelectsAreaFrame code because we need to not call SetSuppressScrolbarrUpdate() in that case.  :(

Comment 29

[kjdixo]

This bug does not seem to have been fixed.
I am experiencing the exact same problem you describe - in Firefox 3.0.16 and Epiphany 2.24.1 (both in Xubuntu Intrepid).
Drop down menus at the tops of web pages unfurl upwards instead of dropping down (as they do so correctly in Opera).
There is an important usability issue here, because if the browser window is maximised on the screen - then most of the menu items are going to be chopped off and lost 'above the screen'. This is extremely annoying as it requires resizing or moving of the browser window every time.
This constant annoyance prompted me track down this bug report.
I hope it is fixable. Thanks.
Kevin Dixon.

Comment 32

[Scott Chapman]

I am seeing this bug also on Ubuntu 10.04 running the Firefox 3.x and 8.0, and on Windows XP in Firefox 8.0.  I have a 1024x600 screen and increased font-size & padding for <options>. If I have more than 6 or 7 options, depending on where the <select> is located on the screen, I get this behavior.  See bug 707975 for details on this situation, including HTML and screen shot.

Comment 33

[Mats Palmgren (inactive)]

Fixed by bug 726264 in the latest Nightly (Fx15).
Will likely also be merged to Fx14.  Possibly also ESR.

Comment 34

[Mats Palmgren (inactive)]

The patch in bug 726264 / bug 575294 caused some problems so it was backed out.
I'm working on it - the plan is to land for Fx16 and merge to Fx15.

Comment 35

[Mats Palmgren (inactive)]

The patches in bug 726264 / bug 575294 landed and fixed this bug.

(note that some of the combobox buttons looks weird in Testcase #3
on OSX -- filed as bug 774128)