Closed
Bug 392629
Opened 17 years ago
Closed 17 years ago
Comboboxes basically unusable with keyboard since border rewrite
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: roc)
References
Details
(Keywords: dogfood, perf, regression)
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
roc
:
approval1.9-
|
Details | Diff | Splinter Review |
I finally decided to try to figure out why combobox performance is so bad...
BUILD: Current trunk Linux build
STEPS TO REPRODUCE:
1) Load this bug page
2) Click on the "Component" combobox so it drops down
3) Try to scroll with keyboard
ACTUAL RESULTS: Very laggy scrolling. Over here, it takes about at least 500ms per option.
EXPECTED RESULTS: Fast scrolling
REGRESSION RANGE: Between 2007-05-29-01 and 2007-05-30-01
MORE INFORMATION:
"top" shows X taking up a lot of time. A profile shows more or less the same (lots of lag when we ask X for mouse coord positions). A profile with --sync shows that most (70%+) of the time is spent under FillFastBorderPath with the following stack (growing down):
FillFastBorderPath
gfxContext::Fill
INT__moz_cairo_fill_preserve
_cairo_gstate_fill
_cairo_surface_fill
_cairo_surface_fallback_fill
_clip_and_composite_trapezoids
_clip_and_composite
_composite_traps_draw_func
_cairo_surface_composite_trapezoids
XRenderCompositeTrapezoids
This actually interferes with daily Bugzilla usage pretty significantly for me...
Flags: blocking1.9?
Comment 1•17 years ago
|
||
Does this interfere with down-arrow scrolling, typing letters to select an item, or both?
Reporter | ||
Comment 2•17 years ago
|
||
Both.
Reporter | ||
Comment 3•17 years ago
|
||
Adding
Option "XAANoOffscreenPixmaps" "true"
to the Device section in xorg.conf helps some. Things are about twice as fast as without that (so still about 2-3 times slower than they used to be). Trying to scroll multiple options by holding down the arrow key still lags out severely...
Depends on: 392934
Reporter | ||
Comment 4•17 years ago
|
||
I tried digging into this today because this is still seriously interfering with basic Bugzilla usage for me. It looks like we're just painting the borders for the nsListControlFrame and some nsBoxFrames. I really don't understand why painting the solid border of nsListControlFrame needs to completely peg the CPU....
Reporter | ||
Comment 5•17 years ago
|
||
I filed bug 397303 on the underlying border-painting issue. roc said he'd look into fixing this by removing/fixing the invalidates in nsListControlFrame to not invalidate the border.
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Assignee | ||
Comment 6•17 years ago
|
||
This should fix it. Hopefully the patch is self-explanatory.
I tried a version where we invalidate just the focus-ring area, but that got a bit more complicated because we have to be able to invalidate before the state that PaintFocus depends on is changed, so I decided it wasn't worth it. That path did lead me to some cleanup of PaintFocus, which I'll attach as a separate patch.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #282187 -
Flags: superreview?(bzbarsky)
Attachment #282187 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•17 years ago
|
||
This is the cleanup ... it conflicts with the previous patch a bit so I'll finish it off after the invalidation fix has landed.
Reporter | ||
Comment 8•17 years ago
|
||
Comment on attachment 282187 [details] [diff] [review]
invalidation fix
>Index: layout/forms/nsListControlFrame.h
>+ * If this frame IsFocused(), invaliates an area that includes anything
"invalidates"/
Document that this needs to happen whenever mEndSelectionIndex changes?
r+sr=bzbarsky with that. Thanks for fixing this! This will make Bugzilla so much more pleasant to use...
Attachment #282187 -
Flags: superreview?(bzbarsky)
Attachment #282187 -
Flags: superreview+
Attachment #282187 -
Flags: review?(bzbarsky)
Attachment #282187 -
Flags: review+
Assignee | ||
Comment 9•17 years ago
|
||
checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 10•17 years ago
|
||
here's the cleanup that I had, updated to trunk. Not sure if I should try to get this into 1.9, but it's done, so might as well land it sometime.
Attachment #282188 -
Attachment is obsolete: true
Attachment #282618 -
Flags: superreview?(bzbarsky)
Attachment #282618 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 11•17 years ago
|
||
Comment on attachment 282618 [details] [diff] [review]
cleanpu
>+ * @param aFocusedContent receives a regular pointer to the focused content
I think we should make it clearer that this does not get addrefed. Maybe even return a rect-and-content struct or something?
With that, r+sr=bzbarsky.
Attachment #282618 -
Flags: superreview?(bzbarsky)
Attachment #282618 -
Flags: superreview+
Attachment #282618 -
Flags: review?(bzbarsky)
Attachment #282618 -
Flags: review+
Assignee | ||
Comment 12•17 years ago
|
||
Comment on attachment 282618 [details] [diff] [review]
cleanpu
I'm going to minus this so I can track it
Attachment #282618 -
Flags: approval1.9-
You need to log in
before you can comment on or make changes to this bug.
Description
•