32920 - [FIX]Extremely slow dropdown with large <select>s

Status: VERIFIED FIXED

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

Description

[Bernhard Rosenkraenzer]

Bugzilla takes forever (and probably hangs at some point) when trying to access
http://bugzilla.redhat.com/bugzilla/query.cgi.
It looks like it's related to the rather huge <select>s used in there; but I'm
not 100% sure of that (didn't have the time to debug this).
Seems some image from the top of the page is getting redrawn 1000s of times.

Comment 1

[Chris Siebenmann]

 I'm not sure if I should be confirming this bug or opening a new
one. What happens for me in a current CVS build (source pulled just
before the tree opened today) is that the page renders very slowly:
first the top area (with the red Redhat logo appears) and the rest
of the page downloads; then it paints what appears to be mispositioned
forms controls or areas with the top left area of the page replicated
by them; then it thinks a while; then finally it repaints the real
versions in their proper positions in an eyeblink.

 If the reporter has a slower machine than mine, I can easily see
how my symptoms would match his, or be an evolved version of his
with more efficient forms rendering code.

 This seems more HTML Form Controls to me than HTML Elements, so I'm
pushing it off thataway too.

Comment 2

[rods (gone)]

moving to M17

Comment 3

[ekrock's old account (dead)]

Marking perf.

Comment 4

[ekrock's old account (dead)]

html32 forms UE b.c. nsbeta2 6/1-.

Comment 5

[ekrock's old account (dead)]

Could this be related to bug 32827, "Pull down lists with scrollbars don't 
operate correctly"?

Comment 6

[rods (gone)]

solution would be too high of a risk for nsbeta2

Comment 7

[rods (gone)]

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

Comment 8

[leger]

Putting on [nsbeta2-] radar.  

Comment 9

[rods (gone)]

Comboboxes now dropdown rather quickly, is this still a bug? Or are there other 
issues?

Comment 10

[rods (gone)]

*** This bug has been marked as a duplicate of 27233 ***

Comment 11

[:deanis74]

This isn't a dup of Bug 27233.  27233 refers specifically to JS fill-ins.  This 
bug refers to the performance of large comboboxes in general.  Check out the 
example from bug 34077, which eventually traces back to this bug:

http://www.unclaimeddomains.com/freesample.html

Dropping down that combobox on my PII-333 can still take up to five seconds at 
100% CPU.  That's not acceptable, especially since it's instantaneous on 4.x.

Someone please a) re-open this bug and b) change the OS to all, as I run WinNT 
and 
experience this.

Comment 12

[:deanis74]

The performance of the combobox on the sample page 
(http://www.unclaimeddomains.com/freesample.html) is still not acceptable.  This 
bug needs to be re-opened.

Comment 13

[bsharma]

Updating QA contact.

Comment 14

[:deanis74]

Based on my comments from July 12, and given that the freesample.html page from 
those comments still takes about five seconds to drop down (2000092608), I'm 
re-opening this.

Comment 15

[rods (gone)]

The problem is the rule that is nede to display focus causes a ReResolveStyle to 
be called on the combobox which ends up ReRsolvingStyle on all the options. If 
there are more than a small number of option the dropdown time gets worse and 
worse.

:focus:-moz-display-comboboxcontrol-frame {
  background-color: Highlight;
  color: HighlightText;
  -moz-outline: 1px dotted HighlightText;
}

Currently, the combobox contains code to draw in the focus rect, because we 
didn't have any way to do it via style. Now that we can do it thru style it is 
too slow. I think in the next release we can find away to optimize it.

In the mean time, it can be fixed by removing the rule above. and adding a call 
to "Invalidate" in the SetFocus method of the Combobox. Here are the diffs:

Index: html.css
===================================================================
RCS file: /cvsroot/mozilla/layout/html/document/src/html.css,v
retrieving revision 3.109
diff -u -r3.109 html.css
--- html.css    2000/09/21 10:20:18     3.109
+++ html.css    2000/09/27 19:36:25
@@ -505,12 +505,6 @@
   -moz-user-select: none;
 }

-:focus:-moz-display-comboboxcontrol-frame {
-  background-color: Highlight;
-  color: HighlightText;
-  -moz-outline: 1px dotted HighlightText;
-}
-
 option {
   min-height: 1em;
   display: block;

Index: nsComboboxControlFrame.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/forms/src/nsComboboxControlFrame.cpp,v
retrieving revision 1.148
diff -u -r1.148 nsComboboxControlFrame.cpp
--- nsComboboxControlFrame.cpp  2000/09/09 22:15:57     1.148
+++ nsComboboxControlFrame.cpp  2000/09/27 19:40:45
@@ -487,7 +487,10 @@
 void
 nsComboboxControlFrame::SetFocus(PRBool aOn, PRBool aRepaint)
 {
-  //XXX: TODO Implement focus for combobox.
+  // Thiis is needed on a temporary basis. It causes the focus
+  // rect to be drawn. This is much faster than ReResolvingStyle
+  // Bug 32920
+  Invalidate(mPresContext, nsRect(0,0,mRect.width,mRect.height), PR_FALSE);
 }

 void

Nominating for RTM because the performance it so bad with large selects. Low 
risk fix.

Comment 16

[rods (gone)]

Summary of bug:
The problem is the rule that is needed to display focus causes a ReResolveStyle 
to be called on the combobox which ends up calling ReResolvingStyle on all the 
options. If there are more than a small number of option the dropdown time gets 
worse and worse.

On the testcase: http://www.unclaimeddomains.com/freesample.html is take 8 
seconds for the combobox to dropdown. With this fix it drops down almost 
immediately, I also notice a speed up on shorter lists also.

Summary of patch:
Basically, the nsComboxControlFrame will track whether it has focus or not and 
either draw a dotted rect to indicate focus or a solid rect to remove it. 
Instead of each frame tracking whether it has focus the nsComboboxControlFrame 
has a static class data member because only one combo frame can have focus at 
any one time.

SetFocus needs to get called on the combo frame for both gaining and losing 
focus and then it does an Invalidateto make sure it gets "painted in". the 
SetFocus was already being called in the event state manager, but when a blur 
event occurred code needed to be added to nsHTMLSelectElement to call SetFocus 
with false.

lower risk - big performace gain.

Comment 17

[rods (gone)]

Attachment: patch file

Comment 18

[Kevin McCluskey (gone)]

Marking rtm+. Low risk fix which fixes extremely bad performance problem.

Comment 19

[Kevin McCluskey (gone)]

Really marking rtm+.

Comment 20

[buster]

changing severity to major, priority to P2.  Without this fix, some pages are
nearly unusable.  I'm in the process of reviewing now, so far, so good.

Comment 21

[buster]

I don't get this part of the diff:
 NS_NewComboboxControlFrame(nsIPresShell* aPresShell, nsIFrame** aNewFrame,
PRUint32 aStateFlags)
 {
@@ -248,6 +251,7 @@
   mItemDisplayWidth             = 0;

   mGoodToGo = PR_FALSE;
+  mFocused  = nsnull;


Why is mFocused set to null in the combobox factory method?  What would happen
if the user clicked on a combobox and somehow the DOM caused a new combobox to
get created on the page?  Does focus get yanked from the first combobox in this
case? If so, wouldn't the blur message null out the arg for us.  And if not,
won't we have the case where the combobox has focus, but now paints as if it
does not?

Comment 22

[rods (gone)]

Steve, this is why we have reviews. You are right, this line was left over from 
when I had it be an instance variable and forgot to remove it. Consider it 
removed and I now am initializing the static class variable like this:

nsComboboxControlFrame * nsComboboxControlFrame::mFocused = nsnull;

Sorry, that should take care of it.

Comment 23

[buster]

with that change, a=buster

Comment 24

[Marc Attinasi]

r=attinasi (with the fix for the class-variable initialization)

Comment 25

[Michael La Guardia]

Marking rtm++.  Please check this in as soon as possible.

Comment 26

[rods (gone)]

fixed

Comment 27

[bsharma]

Build: 2000-10-08-09 MN6
Platform: Win98

Comment 28

[jmraker]

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