1000875 - Opaque fixed background repainted when starting to scroll

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

Attachment: 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.

Comment 1

[Markus Stange [:mstange]]

Attachment: part 1: Give nsDOMWindowUtils::CheckAndClearPaintedState the ability to check paints of canvas frames.

Comment 2

[Markus Stange [:mstange]]

Attachment: part 2: Improve test_fixed_bg_scrolling_repaints.html.

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.

Comment 3

[Markus Stange [:mstange]]

Attachment: part 3: Add invalidation debugging annotations for layer content type changes.

Invalidation debugging doesn't print the reason for the invalidation in the testcase. This adds the annotation for one of the reasons.

Comment 4

[Markus Stange [:mstange]]

Attachment: part 4: Add invalidation debugging annotations for changes of a layer's forced background color.

This adds the other.

Comment 5

[Markus Stange [:mstange]]

Attachment: part 5: Fix calculation of opaque region of the background image display item

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.

Comment 6

[[github robot]]

Commit pushed to master at https://github.com/mozilla/webmaker.org

https://github.com/mozilla/webmaker.org/commit/a0f3e67d2e8f19372b16945eaba83254a47b0cc4
Fix bug 1000875 - Style tweaks for badges

Comment 7

[Timothy Nikkel (:tnikkel)]

That robot has got the wrong bug number.

Comment 8

[inactivate account for kate]

Sorry about that guys

Comment 9

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

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?

Comment 10

[Markus Stange [:mstange]]

(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?

Comment 11

[Markus Stange [:mstange]]

see comment 10

Comment 12

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

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

Comment 13

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c893efb5eb7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c668dc3e1826
https://hg.mozilla.org/integration/mozilla-inbound/rev/6607fab12f18
https://hg.mozilla.org/integration/mozilla-inbound/rev/baaaf70d2bca
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd797f07d0db

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8c893efb5eb7
https://hg.mozilla.org/mozilla-central/rev/c668dc3e1826
https://hg.mozilla.org/mozilla-central/rev/6607fab12f18
https://hg.mozilla.org/mozilla-central/rev/baaaf70d2bca
https://hg.mozilla.org/mozilla-central/rev/fd797f07d0db

Comment 15

[Markus Stange [:mstange]]

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

Comment 16

[Timothy Nikkel (:tnikkel)]

Linux too! Although much smaller.

Comment 17

[Markus Stange [:mstange]]

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.