Closed
Bug 1255214
Opened 8 years ago
Closed 8 years ago
Scrollbar button is constantly invalidated
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: botond, Assigned: botond)
Details
Attachments
(1 file)
Markus and I discovered this bug while trying to reproduce bug 1255068 on my Linux machine. STR: 1. Add the following to the file ~/.config/gtk3.0/gtk.css (create the file if it doesn't exist): .scrollbar { -GtkScrollbar-has-backward-stepper: true; -GtkScrollbar-has-forward-stepper: true; } This overrides the default GTK3 style, causing scrollbars to have scrollbar buttons. 2. Run firefox and turn on paint flashing. 3. Scroll any page with a scrollbar. Observe how the scrollbar buttons are constantly invalidated and repainted. The repaints caused by the scrollbar buttons are masking bug 1255068 (which requires infrequent paints to be visible).
Assignee | ||
Comment 2•8 years ago
|
||
diagnosis |
Markus and I investigated this a bit more. The problem is that nsNativeThemeGTK::WidgetStateChanged() is telling us we should repaint the scrollbar button widget every time the "curpos" attribute changes [1], even though it's only actually necessary to repaint when the thumb reaches, or moves away from, one of the ends of the track (because the button becomes disabled when the thumb reaches the corresponding end of the track). Sketch of a fix: - Change nsITheme::WidgetStateChanged() to optionally take the old value of the attribute being changed as an extra parameter. - In nsNativeThemeGTK::WidgetStateChanged(), use the extra parameter to determine whether the button should change enablement, reproducing the enablement-determining logic from GetGtkWidgetAndState(), and only set aShouldRepaint if it should. I'll implement this when I get a chance, but it's not high priority. [1] https://dxr.mozilla.org/mozilla-central/rev/f0c0480732d36153e8839c7f17394d45f679f87d/widget/gtk/nsNativeThemeGTK.cpp#1674
Assignee | ||
Comment 3•8 years ago
|
||
I'm going to fix this because the constant repainting caused by the scrollbar button is making it more difficult for me to see paint-skipping issues.
Assignee: nobody → botond
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47911/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47911/
Attachment #8743599 -
Flags: review?(mstange)
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e86edb82d2d9
Comment 6•8 years ago
|
||
Comment on attachment 8743599 [details] MozReview Request: Bug 1255214 - Only repaint GTK scrollbar button if its enablement actually changed. r=mstange https://reviewboard.mozilla.org/r/47911/#review44739 ::: gfx/src/nsITheme.h:133 (Diff revision 1) > virtual Transparency GetWidgetTransparency(nsIFrame* aFrame, uint8_t aWidgetType) > { return eUnknownTransparency; } > > NS_IMETHOD WidgetStateChanged(nsIFrame* aFrame, uint8_t aWidgetType, > - nsIAtom* aAttribute, bool* aShouldRepaint)=0; > + nsIAtom* aAttribute, bool* aShouldRepaint, > + const nsAttrValue* aOldValue = nullptr)=0; I'd prefer it if you didn't make this a default argument and just passed nullptr at the one remaining caller.
Attachment #8743599 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8743599 [details] MozReview Request: Bug 1255214 - Only repaint GTK scrollbar button if its enablement actually changed. r=mstange Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47911/diff/1-2/
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #6) > I'd prefer it if you didn't make this a default argument and just passed > nullptr at the one remaining caller. Fixed.
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9f69fc58466e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•