Closed
Bug 1068961
Opened 10 years ago
Closed 10 years ago
ColorLayer not fully rendered when zoomed
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
VERIFIED
FIXED
mozilla35
People
(Reporter: botond, Assigned: botond)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
roc
:
review+
lmandel
:
approval-mozilla-beta+
bajaj
:
approval-mozilla-b2g34+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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).
Assignee | ||
Comment 2•10 years ago
|
||
The problem does not occur with the layout.async-containerless-scrolling.enabled pref set to false, making this a multi-layer-apz regression.
Blocks: multi-layer-apz
Keywords: regression
Assignee | ||
Comment 3•10 years ago
|
||
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
Assignee: nobody → botond
Attachment #8493961 -
Flags: feedback?(tnikkel)
Comment 4•10 years ago
|
||
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.
Attachment #8493961 -
Flags: review?(roc)
Attachment #8493961 -
Flags: feedback?(tnikkel)
Attachment #8493961 -
Flags: feedback+
Attachment #8493961 -
Flags: review?(roc) → review+
Assignee | ||
Comment 5•10 years ago
|
||
failure try |
Assignee | ||
Comment 6•10 years ago
|
||
(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.
Assignee | ||
Comment 7•10 years ago
|
||
green try |
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
Attachment #8493961 -
Attachment is obsolete: true
Attachment #8495991 -
Flags: review?(roc)
Comment 8•10 years ago
|
||
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.
Attachment #8495991 -
Flags: review?(roc) → review+
Assignee | ||
Comment 9•10 years ago
|
||
landing |
Comment 10•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 11•10 years ago
|
||
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
Attachment #8495991 -
Flags: approval-mozilla-b2g34?
Comment 12•10 years ago
|
||
Comment on attachment 8495991 [details] [diff] [review]
Reset the clip rect for color layers in FrameLayerBuilder
Approving as this is needed for 1062792
Attachment #8495991 -
Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Comment 13•10 years ago
|
||
status-b2g-v2.1:
--- → fixed
status-b2g-v2.2:
--- → fixed
status-firefox33:
--- → wontfix
status-firefox34:
--- → wontfix
status-firefox35:
--- → fixed
Comment 14•10 years ago
|
||
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
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
Attachment #8495991 -
Flags: approval-mozilla-beta?
Comment 16•10 years ago
|
||
Comment on attachment 8495991 [details] [diff] [review]
Reset the clip rect for color layers in FrameLayerBuilder
Beta+
Attachment #8495991 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•10 years ago
|
tracking-firefox34:
--- → +
Comment 17•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•