Opaque fixed background repainted when starting to scroll

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

Trunk
mozilla32
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox32 disabled)

Details

Attachments

(6 attachments)

Posted file testcase
Load the testcase and enable paint flashing. Then scroll down a little, scroll back all the way up, and repeat. Each time the scroll position changes between zero and non-zero, the background image is repainted.
With a few improvements this test would have caught this bug.

The most important change here is testing document.documentElement instead of document.body. The addition of overflow:hidden doesn't really change anything because the mochitest runner runs the test in an iframe with scrolling="no", but at least it makes it explicit. And we actually can't have any scrollbars in this testcase because then we'd detect scrollbar painting and the test would fail.
Attachment #8411801 - Flags: review?(roc)
Invalidation debugging doesn't print the reason for the invalidation in the testcase. This adds the annotation for one of the reasons.
Attachment #8411803 - Flags: review?(roc)
And this patch actually fixes the bug.

We were moving the clip rect of the opaque region of the background image display item along with scrolling. This patch disables the clip in the same way that GetBoundsInternal does it - by special casing canvas frames and using the whole canvas area for them.
Attachment #8411805 - Flags: review?(roc)
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
That robot has got the wrong bug number.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry about that guys
Comment on attachment 8411797 [details] [diff] [review]
part 1: Give nsDOMWindowUtils::CheckAndClearPaintedState the ability to check paints of canvas frames.

Review of attachment 8411797 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMWindowUtils.cpp
@@ +3041,5 @@
> +  // canvasframe invalidations by observing the documentElement.
> +  for (;;) {
> +    nsIFrame* parentFrame = frame->GetParent();
> +    if (parentFrame && parentFrame->GetContent() == content) {
> +      frame = parentFrame;

How about we call CheckAndClearPaintedState on every frame between the primary frame and the outermost frame for the element, and OR the results?
Attachment #8411797 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (PTO April 25 to May 3) from comment #9)
> Comment on attachment 8411797 [details] [diff] [review]
> part 1: Give nsDOMWindowUtils::CheckAndClearPaintedState the ability to
> check paints of canvas frames.
> 
> Review of attachment 8411797 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsDOMWindowUtils.cpp
> @@ +3041,5 @@
> > +  // canvasframe invalidations by observing the documentElement.
> > +  for (;;) {
> > +    nsIFrame* parentFrame = frame->GetParent();
> > +    if (parentFrame && parentFrame->GetContent() == content) {
> > +      frame = parentFrame;
> 
> How about we call CheckAndClearPaintedState on every frame between the
> primary frame and the outermost frame for the element, and OR the results?

Sure, but I don't really understand why that would be better. nsFrame::CheckAndClearPaintedState already recurses over its children:
http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#245-262
Are there cases where this may not traverse the primary frame of the element?
see comment 10
Flags: needinfo?(roc)
Comment on attachment 8411797 [details] [diff] [review]
part 1: Give nsDOMWindowUtils::CheckAndClearPaintedState the ability to check paints of canvas frames.

Review of attachment 8411797 [details] [diff] [review]:
-----------------------------------------------------------------

right
Attachment #8411797 - Flags: review- → review+
Looks like this gave a healthy speedup on tscroll-ASAP, at least on Mac (~20%):

https://groups.google.com/d/topic/mozilla.dev.tree-management/NJXBytpD2UY/discussion
Linux too! Although much smaller.
Depends on: 1011166
Depends on: 1010188
Backed out on Beta for causing bug 1011166 (see the description in bug 1011166 comment 40):
https://hg.mozilla.org/releases/mozilla-beta/rev/11a5306111d0

This might trigger a few performance regression warnings on Beta.
Depends on: 1176724
Depends on: 1271112
No longer depends on: 1176724
You need to log in before you can comment on or make changes to this bug.