Closed Bug 392629 Opened 17 years ago Closed 17 years ago

Comboboxes basically unusable with keyboard since border rewrite

Categories

(Core :: Graphics, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: roc)

References

Details

(Keywords: dogfood, perf, regression)

Attachments

(2 files, 1 obsolete file)

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?
Does this interfere with down-arrow scrolling, typing letters to select an item, or both?
Both.
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...
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....
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.
Flags: blocking1.9? → blocking1.9+
Attached patch invalidation fix (deleted) — Splinter Review
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)
Attached patch cleanup (obsolete) (deleted) — Splinter Review
This is the cleanup ... it conflicts with the previous patch a bit so I'll finish it off after the invalidation fix has landed.
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+
checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Attached patch cleanpu (deleted) — Splinter Review
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)
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+
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.

Attachment

General

Created:
Updated:
Size: