802440 - Some elements are invalidated when other elements are scrolled off the screen

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jonathan Watt [:jwatt]]

Attachment: testcase

Related to bug 802321, and with the two patches from that bug applied, I'm seeing invalidation of transformed elements when scrolling down as the items are scrolled off the top of the screen. Specifically, in this testcase, as the bottom of the top circle is just disappearing off/appearing on the screen, we seem to invalidate an area bounding all five circles. Scrolling up and down by about one pixel at that position shows the bug. It seems really undesirable to be invalidating content that is scrolling _off_ the screen.

This testcase is a reduction of more severe invalidation that I'm seeing in http://www.jasondavies.com/factorisation-diagrams/ with the two patches from bug 802321 applied. If the fix for this testcase doesn't fix invalidation of the top/bottom rows when scrolling them on/off screen let me know and I'll make more reduced testcases.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: simpler testcase

Comment 2

[Jonathan Watt [:jwatt]]

With this testcase it's pretty clear that when the first line is scrolled out of view we invalidate the area of the second line.

Comment 3

[Jonathan Watt [:jwatt]]

The problem is that ContainerLayerProperties::ComputeChangeInternal will invalidate children at a given index if they are not the same object in both the old and new list. As a result, when we scroll an item off the screen (causing it to be removed from the new list), then all subsequent siblings will be invalidated.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 6

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

Comment on attachment 683822 [details] [diff] [review]
patch

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

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +223,4 @@
>      for (Layer* child = container->GetFirstChild(); child; child = child->GetNextSibling()) {
> +      bool invalidateChildsCurrentArea = false;
> +      if (i < mChildren.Length()) {
> +        uint32_t childs_old_index;

childsOldIndex

Comment 7

[Jonathan Watt [:jwatt]]

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/8dafa8b0a2b9

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8dafa8b0a2b9

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 683822 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 539356
User impact if declined: performance regressions, particularly when scrolling
Testing completed (on m-c, etc.): baked on m-c for several days
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

Comment 10

[Alex Keybl [:akeybl]]

Comment on attachment 683822 [details] [diff] [review]
patch

DLBI regression - let's land on branches.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/77ce88901a93
https://hg.mozilla.org/releases/mozilla-beta/rev/04ef8e9f11a9

Comment 12

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0

Verified as fixed based on test cases from comment 0 and comment 1 on Firefox 20.0 (buildId: 20130326150557)