Closed Bug 1255214 Opened 4 years ago Closed 4 years ago

Scrollbar button is constantly invalidated

Categories

(Core :: Layout, defect)

Unspecified
Linux
defect
Not set

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).
I don't see the buttons paint-flashing on Windows.
OS: Unspecified → Linux
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
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
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+
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/
(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.
https://hg.mozilla.org/mozilla-central/rev/9f69fc58466e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.