1068961 - ColorLayer not fully rendered when zoomed

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

STR:
  1. Load http://people.mozilla.org/~bballo/square-color.html in the B2G browser.
  2. Zoom in.
  3. Scroll to the bottom-left of the subframe, and then scroll up.

Expected results:
  The scrolled content is all green.

Actual results:
  Only part of the scrolled content is green, the rest is white.

The problem does not occur with http://people.mozilla.org/~bballo/square.html, which adds some text to force a Thebes layer rather than a Color layer. This makes me suspect that the problem might be related to bug 1066713.

Comment 1

[Benoit Girard (:BenWa)]

ColorLayerComposite.cpp just uses mBounds. I'd look there first. A quick comparison against ThebesLayerComposite.cpp and that uses the EffectiveVisibleRegion. This could explain this problem (if it does not I'd test color layers under a intermediate surface container layer).

Comment 2

[Botond Ballo [:botond]]

The problem does not occur with the layout.async-containerless-scrolling.enabled pref set to false, making this a multi-layer-apz regression.

Comment 3

[Botond Ballo [:botond]]

Attachment: Reset the clip rect for color layers in FrameLayerBuilder

I debugged this with Timothy's help. The problem is that the clip rect of the color layer does not increase as the page is zoomed in.

The reason this doesn't happen with Thebes layers is that FrameLayerBuilder resets the clip rect of Thebes layers (but not Color layers) in PopThebesLayerData(). 

SetupScrollingMetadata() then sees that the layer already has a clip rect, and intersects the existing (pre-zoom, and thus smaller) clip rect with the one computed by ComputeFrameMetrics() (which is post-zoom and thus larger) [1]. As a result, the clip rect fails to grow as we zoom.

Modifying FrameLayerBuilder to clear the clip rect for Color layers too in PopThebesLayerData() seems to fix the problem.

Timothy, is this what you had in mind?

[1] https://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=8252eae8278c#3517

Comment 4

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8493961 [details] [diff] [review]
Reset the clip rect for color layers in FrameLayerBuilder

Yeah, I think the problem was that we never had clip rects on color layers before multi layer apzc. Now that we do we need to clear the old clip rect.

Comment 5

[Botond Ballo [:botond]]

https://tbpl.mozilla.org/?tree=Try&rev=f58c80c60240

Comment 6

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #5)
> https://tbpl.mozilla.org/?tree=Try&rev=f58c80c60240

The Try push has some async-scrolling reftest failures that look like they could be caused by this patch.

Comment 7

[Botond Ballo [:botond]]

Attachment: Reset the clip rect for color layers in FrameLayerBuilder

The problem was that if ThebesLayerData::mItemClip had a clip set, the patch used that clip rather than clearing the clip. However, for a ColorLayer this is the incorrect clip to use. The solution is to always just clear the clip for ColorLayers.

Clean Try push: https://tbpl.mozilla.org/?tree=Try&rev=97cb4d8c05d7

Comment 8

[Timothy Nikkel (:tnikkel)]

The problem occurs because there can be two types of clips: those that are induced by a containing scroll frame, and all other clips. The first type doesn't move with the content, the second does. SetupScrollingMetaData does the first, everything else is the second. When APZC moves a layer it doesn't move the clip, so this can only work if the clip on the layer is of the first type. And intersecting a clip of the first and the second type will also produce this problem.

The reason this only affects color layers is because thebes layers usually don't get a clip because the items in the thebes layer usually don't share the same clip, so the layer doesn't get a clip, they just get clipped individually when drawn. Image layers do get a clip because they contain one item, but we use the ASSUME_DRAWING_RESTRICTED_TO_CONTENT_RECT flag to skip unnecessary clips so that the image display item will usually not have a clip.

So I think there is still an underlying bug where a thebes layer could get a non-scroll induced clip, it just doesn't happen very often.

Comment 9

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/62e308af0e0d

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62e308af0e0d

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8495991 [details] [diff] [review]
Reset the clip rect for color layers in FrameLayerBuilder

We need this in order to make the landing of bug 1062792 on b2g34 green.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 967844
User impact if declined: wrong rendering in some cases
Testing completed: been on m-c for a while now
Risk to taking this patch (and alternatives if risky): not very risky
String or UUID changes made by this patch: none

Comment 12

[bhavana bajaj [:bajaj]]

Comment on attachment 8495991 [details] [diff] [review]
Reset the clip rect for color layers in FrameLayerBuilder

Approving as this is needed for 1062792

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/226176645811

Comment 14

[Brogan Zumwalt [Inactive]]

Issue verified fixed on Flame 2.1 and Flame 2.2

Actual Results: All parts of scrolling colored square are green.

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141104001202
Gaia: 8b0cf889ae0d48a9eb7ecdcb9b67590de45cc5e5
Gecko: 388b03efe92d
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141104040207
Gaia: 3c50520982560ccba301474d1ac43706138fc851
Gecko: 54d05732f29b
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Comment 15

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

Comment on attachment 8495991 [details] [diff] [review]
Reset the clip rect for color layers in FrameLayerBuilder

Approval Request Comment
Bug caused by (feature/regressing bug #): bug 967844
User impact if declined: wrong rendering in some cases
Testing completed: been on m-c for a while now
Risk to taking this patch (and alternatives if risky): not very risky
String or UUID changes made by this patch: none

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8495991 [details] [diff] [review]
Reset the clip rect for color layers in FrameLayerBuilder

Beta+

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9f14f2af8bf7