500632 - Unnecessary repainting due to invalidation inside collapsed scrollbars

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Scrollframes collapse scrollbars by setting their size to 0. However, the thumb and other scrollbar parts retain their preferred size. When something causes scrollbar parts to be invalidated, those invalidations propagate out and cause actual repainting. The simplest way I found to stop that from happening was to explicitly suppress invalidations from collapsed scrollbars in nsHTMLScrollFrame::InvalidateInternal.

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 385355 [details] [diff] [review]
fix

r=dbaron

However, please document the INVALIDATE_NOTIFY_ONLY flag in nsIFrame.h.  (It originated in bug 450930.)  (I'm actually not sure why nsGfxScrollFrame::InvalidateInternal currently checks for it.)

Also, if you want (feel free to do this or not), it seems like it might make sense to structure nsGfxScrollFrame::InvalidateInternal as:

if (aForChild == mInner.mScrolledFrame) {
  // ... existing code, inside if (!(aFlags & INVALIDATE_NOTIFY_ONLY))
} else if (aForChild == mInner.mHScrollbarBox) {
  // your conditional new early return, with your comment
} else if (aForChild == mInner.mVScrollbarBox) {
  // same, but with pointer to previous comment
}

nsHTMLContainerFrame::InvalidateInternal(...);

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #1)
> (From update of attachment 385355 [details] [diff] [review])
> r=dbaron
> 
> However, please document the INVALIDATE_NOTIFY_ONLY flag in nsIFrame.h.  (It
> originated in bug 450930.)  (I'm actually not sure why
> nsGfxScrollFrame::InvalidateInternal currently checks for it.)

Hmm ... actually I think it's not necessary. I'll file a separate bug to get rid of it.

> Also, if you want (feel free to do this or not), it seems like it might make
> sense to structure nsGfxScrollFrame::InvalidateInternal as:
> 
> if (aForChild == mInner.mScrolledFrame) {
>   // ... existing code, inside if (!(aFlags & INVALIDATE_NOTIFY_ONLY))
> } else if (aForChild == mInner.mHScrollbarBox) {
>   // your conditional new early return, with your comment
> } else if (aForChild == mInner.mVScrollbarBox) {
>   // same, but with pointer to previous comment
> }
> 
> nsHTMLContainerFrame::InvalidateInternal(...);

Good idea, I'll do that.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/916be128c4ff

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Filed bug 505667 on the first part of comment #2.