1100756 - Classic scrollbars don't draw if they overlap a scrollframe

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: layerdump.txt

If you enable apzc and shrink a window with classic scrollbars, you eventually lose the scrollbars. The layers for the scrollbars are in the right place, but the actual layer with content draws over them. Sample layer dump included.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: bug1100756-clip.patch

Comment 2

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8526458 [details] [diff] [review]
bug1100756-clip.patch

Actually this isn't conditional on containerless scrolling. If the clip is right is should be applicable for containerless and containerful.

Also, is there a bug to make the composition bounds calculation use the scroll port instead of the frame rect?

Comment 3

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8526458 [details] [diff] [review]
bug1100756-clip.patch

I've looked into and I still do not understand why this should be different when using containers for root scrolling vs not using containers. If it works for one it should work just as well for the other. The clip rect computed in ScrollFrameHelper::ComputeFrameMetrics gets put on the thebes layers in both cases (not the container layer).

I also do not see how the computed clip rect can be correct. For the root the composition size comes from the widget bounds. So it does not change as we zoom in. But when we use the clip in ContainerState::SetupScrollingMetadata we apply ScaleToNearestPixels to it first, which multiplies by the resolution. So the clip rect is going to get bigger as we zoom in; it should be staying the same size.

Or am I missing something obvious?

Comment 4

[Timothy Nikkel (:tnikkel)]

Maybe we should just always make scroll layers fully visible? ie skip occlusion analysis of them in frame layer builder.

Comment 5

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8526458 [details] [diff] [review]
bug1100756-clip.patch

I think we should do comment 4 instead of this.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Don't we need the correct clips applied for composition as well as for the occlusion check?

Comment 7

[Timothy Nikkel (:tnikkel)]

The scrollbars are laid out at the desktop scrollport, and then the apzc moves them to the right place. So the clip that makes the scrollbars visible from FrameLayerBuilder's perspective is a different clip from what the correct clip to use for clipping the scrolled content when composited on screen. And the former clip really has no relation to what is drawn on screen.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v2

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8526458 [details] [diff] [review]
bug1100756-clip.patch

Ok, seems like there's no consensus... I'm pretty sure this is the correct clip rect with containerless scrolling, nothing else works and I can't find any cases (yet) where this doesn't.

It's true that this clip also implicitly causes scrollbars to not be culled during PostProcessRetainedLayers(). If we want to make that explicit (it doesn't seem necessary), I have that patch, but that behavior existed before this bug. And even with that, we still need this clip.

Comment 10

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

Comment #4 sounds right to me actually. What's the problem with it?

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

No problem with it really, it's just that we need the clip anyway, and the clip already solves the occlusion problem. But - here's the patch that factors scrollbars in explicitly.

Turns out the clip rect I had was wrong anyway. It should be the scrollport and there's a separate bug with this + tiling that I'll file shortly (tiling does not compute the displayport correctly).

Comment 12

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

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

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

Looks good modulo the comments.

::: gfx/layers/Layers.h
@@ +1204,5 @@
>      }
>    }
>  
> +  // Set during construction for the container layer of scrollbar components.
> +  void SetIsScrollbarRoot()

I'm not sure what "Root" means in this context. Could we call this "SetIsScrollbarContainer"?

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2536,5 @@
>  
>  static void
>  AppendToTop(nsDisplayListBuilder* aBuilder, const nsDisplayListSet& aLists,
> +            nsDisplayList* aSource, nsIFrame* aSourceFrame,
> +            bool aOwnLayer, bool aPositioned, bool aIsScrollbarRoot)

I think the time has come to use an explicit flags word here!

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

w/ comments addressed

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/integration/mozilla-inbound/rev/650de89062f6

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/650de89062f6