Closed Bug 1260335 Opened 8 years ago Closed 8 years ago

Scroll clips are not moved properly with scrolled perspctive and nested scrollframes

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- verified

People

(Reporter: mstange, Assigned: mstange)

References

Details

(Keywords: correctness, Whiteboard: [gfx-noted])

Attachments

(3 files, 1 obsolete file)

testcase
(deleted), text/html
Details
MozReview Request: Bug 1260335 - Add a comment that explains why the perspective child can't have more than one frame metrics. r?mattwoodrow
(deleted), text/x-review-board-request
mattwoodrow
: review+
Details
MozReview Request: Bug 1260335 - On perspective ContainerLayers, the clip deferred from their child layer needs to be affected by the perspective layer's async transforms. r?botond
(deleted), text/x-review-board-request
botond
: review+
ritu
: approval-mozilla-aurora+
Details
Attached patch reftest (obsolete) (deleted) — — Splinter Review 
The attached test does not pass. It should pass.

In this testcase, I have a scrollable root frame and a scrollable subframe. There is a perspctive on the subframe, and a will-change:transform layer inside the subframe.
When APZ scrolls the root frame, the subframe's clip should move and never clip the transformed layer. Only the root frame's clip should clip the transformed layer.
But what happens is that the clip of the subframe stays in place, and clips the transformed layer.
Attached file testcase (deleted) — 
STR with this testcase:
 1. Scroll the subframe down and back up so that it gets a display port.
 2. Move your mouse outside the subframe (but still inside the root frame).
 3. Scroll down the root frame.
If the main thread paints triggered at the right moments, then you'll see the big black border being clipped at the top before it reaches the top of the viewport.
Summary: Scroll clips are not moved properly with scrolled perspctive → Scroll clips are not moved properly with scrolled perspctive and nested scrollframes
Comment on attachment 8736436 [details]
MozReview Request: Bug 1260335 - Add a comment that explains why the perspective child can't have more than one frame metrics. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43289/diff/1-2/
Comment on attachment 8736437 [details]
MozReview Request: Bug 1260335 - On perspective ContainerLayers, the clip deferred from their child layer needs to be affected by the perspective layer's async transforms. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43291/diff/1-2/
Attachment #8735676 - Attachment is obsolete: true
Attachment #8736436 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8736436 [details]
MozReview Request: Bug 1260335 - Add a comment that explains why the perspective child can't have more than one frame metrics. r?mattwoodrow

https://reviewboard.mozilla.org/r/43289/#review39949
Comment on attachment 8736437 [details]
MozReview Request: Bug 1260335 - On perspective ContainerLayers, the clip deferred from their child layer needs to be affected by the perspective layer's async transforms. r?botond

https://reviewboard.mozilla.org/r/43291/#review39937

Nice catch! I actually discussed this issue with Matt at the time [1], and we decided that the deferred clip _should_ be affected by async transforms on the parent layer, but somehow something got lost in translation from that discussion to my implementation.

[1] logs.glob.uno/?c=mozilla%23gfx&s=16+Nov+2015&e=18+Nov+2015#c250193

::: layout/reftests/async-scrolling/perspective-scrolling-4-ref.html:3
(Diff revision 2)
> +<!DOCTYPE html>
> +<html lang="en"
> +     reftest-async-scroll

Does the reference page need reftest-async-scroll annotations at all?

::: layout/reftests/async-scrolling/perspective-scrolling-4.html:41
(Diff revision 2)
> +  height: 4000px;
> +}
> +
> +</style>
> +
> +<div class="scrollbox"

This div is not closed.

::: layout/reftests/async-scrolling/perspective-scrolling-4.html:50
(Diff revision 2)
> +
> +<div class="transformed"></div>
> +<div class="spacer"></div>
> +
> +<script>
> +// document.scrollingElement.scrollTop = 250;

Why are there commented-out lines here?
Attachment #8736437 - Flags: review?(botond) → review+
Is this a regression/follow-up from a previous fix? Wondering if the status-firefoxN flags need to be set for 47/46/etc.
Keywords: correctness
Whiteboard: [gfx-noted]
I think this never worked correctly. The bug was in the original patches that landed with bug 1168263. I'll set the flags.
status-firefox46: --- → affected
status-firefox47: --- → affected
https://hg.mozilla.org/mozilla-central/rev/ff6c2806545e
https://hg.mozilla.org/mozilla-central/rev/4f1bcad3f4d6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment on attachment 8736437 [details]
MozReview Request: Bug 1260335 - On perspective ContainerLayers, the clip deferred from their child layer needs to be affected by the perspective layer's async transforms. r?botond

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: Small APZ correctness issue in a rare case
[Describe test coverage new/current, TreeHerder]: this patch adds a test
[Risks and why]: low, small well-understood patch
[String/UUID change made/needed]: none
Attachment #8736437 - Flags: approval-mozilla-aurora?
This isn't really worth uplifting to Beta, so marking wontfix for 46. We can still uplift it later if doing so would make uplifting other patches easier.
status-firefox46: affected → wontfix
Depends on: 1264092
Comment on attachment 8736437 [details]
MozReview Request: Bug 1260335 - On perspective ContainerLayers, the clip deferred from their child layer needs to be affected by the perspective layer's async transforms. r?botond

APZ related and the patch has a new automated test (Awesome!), Aurora47+
Attachment #8736437 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
If this is landed on Aurora, bug 1264092 needs to land with it, otherwise the test will fail on Aurora.
(In reply to Markus Stange [:mstange] from comment #15)
> If this is landed on Aurora, bug 1264092 needs to land with it, otherwise
> the test will fail on Aurora.

Sure, test only changes don't need approval, let's uplift the patch from bug 1264092 to Aurora 47 too.
Reproduced the clipping on Nightly 48.0a1 2016-03-29, Win 10 64-bit.
In this build, the black border is being clipped at the bottom: http://i.imgur.com/7pN031m.png

This doesn't happen anymore on Aurora 48.0a2 2016-05-22. 

Possible issue: After step 3 from comment 1, scroll up the root frame - sometimes the black border is not shown.

Markus, can you please check if the above notes are expected?
Flags: needinfo?(mstange)
(In reply to Petruta Rasa [QA] [:petruta] from comment #18)
> Possible issue: After step 3 from comment 1, scroll up the root frame -
> sometimes the black border is not shown.

I can reproduce this. It's a bug, and I'm pretty sure it's bug 1262182.
Flags: needinfo?(mstange)
Thanks! Marking 48 as verified.
Status: RESOLVED → VERIFIED
status-firefox48: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: