Closed Bug 951415 Opened 6 years ago Closed 6 years ago

White edge when scrolling

Categories

(Core :: Graphics: Layers, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 935219

People

(Reporter: ajones, Assigned: ajones)

References

()

Details

Attachments

(1 file)

Composition bounds are being update incorrectly and applied incorrectly.

Steps to reproduce:

1. Navigate to daringfireball.net
2. Scroll down
3. Zoom all the way out
4. Scroll left and right

Expected results:
Dark background

Actual results:
White bar on the right of the screen
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Assignee: nobody → ajones
Status: NEW → ASSIGNED
Way to steal my patch. See bug 951320. We should coordinate more about what you're working on so this doesn't happen again.
Comment on attachment 8349051 [details] [diff] [review]
Fix GetExpandedScrollRect() mixed co-ordinate spaces;

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

::: gfx/layers/FrameMetrics.h
@@ +122,5 @@
>    // down. i.e. we know that scrollableRect can go back as far as zero.
>    // but we don't know how much further ahead it can go.
>    CSSRect GetExpandedScrollableRect() const
>    {
> +    CSSRect cssCompositionBounds = mCompositionBounds / mZoom;

This should probably just use CalculateCompositedRectInCssPixels. Feel free to fix bug 948377 while you're at it.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1456,5 @@
>      // in some things into our local mFrameMetrics because these things are
>      // determined by Gecko and our copy in mFrameMetrics may be stale.
>      mFrameMetrics.mScrollableRect = aLayerMetrics.mScrollableRect;
> +    // We only want to use the child's composition bounds if we've no the
> +    // top level APZC.

I don't understand why you made this change. Can you explain?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> Way to steal my patch. See bug 951320. We should coordinate more about what
> you're working on so this doesn't happen again.

I just stumbled across the issue so I thought I'd include the fix in my patch. It is fairly obviously wrong but perhaps it works on metro where the scale is more often 1:1.

> ::: gfx/layers/ipc/AsyncPanZoomController.cpp
> @@ +1456,5 @@
> >      // in some things into our local mFrameMetrics because these things are
> >      // determined by Gecko and our copy in mFrameMetrics may be stale.
> >      mFrameMetrics.mScrollableRect = aLayerMetrics.mScrollableRect;
> > +    // We only want to use the child's composition bounds if we've no the
> > +    // top level APZC.
>
> I don't understand why you made this change. Can you explain?

At the top level APZCTreeManager::UpdateRootCompositionBounds() sets the composition bounds based on the local tree. Right now the composition bounds are being wiped out by a old value being sent back from TabChild when a paint completes. It possibly makes more sense to move all the composition bounds logic inside APZCTreeManager rather than having it split between two classes.
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #4)
> At the top level APZCTreeManager::UpdateRootCompositionBounds() sets the
> composition bounds based on the local tree. Right now the composition bounds
> are being wiped out by a old value being sent back from TabChild when a
> paint completes. It possibly makes more sense to move all the composition
> bounds logic inside APZCTreeManager rather than having it split between two
> classes.

Ah. I just removed UpdateRootCompositionBounds as part of bug 950487. Now the composition bounds is only ever updated by the NotifyLayersUpdated code path.
Your change seems to make the b2g bug worse. I'll have to trace the composition bounds back to see where we're getting the wrong value from.
Yeah, please do. When I removed UpdateRootCompositionBounds it was my understanding based on the code that we should always get the same composition bounds coming in on the next call to NotifyLayersUpdate so if there's a bad value sneaking in somewhere we should track it down.
Note also that there is a bug on file and being worked on to fix the composition bounds calculation in RecordFrameMetrics. bug 935219.
UpdateRootCompositionBounds was giving us the right answer and RecordFrameMetrics (in nsDisplayList.cpp) is giving us the wrong answer. Fixing RecordFrameMetrics should fix this issue.
Bug 935219 is fixed now; is there anything left here that needs to be addressed?
Flags: needinfo?(ajones)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(ajones)
Resolution: --- → DUPLICATE
Duplicate of bug: 935219
You need to log in before you can comment on or make changes to this bug.