Closed Bug 1100756 Opened 10 years ago Closed 10 years ago

Classic scrollbars don't draw if they overlap a scrollframe


(Core :: Panning and Zooming, defect)

Not set





(Reporter: dvander, Assigned: dvander)




(2 files, 3 obsolete files)

Attached file 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.
Attached patch bug1100756-clip.patch (obsolete) — Splinter Review
Attachment #8526458 - Flags: review?(tnikkel)
Comment on attachment 8526458 [details] [diff] [review]

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 on attachment 8526458 [details] [diff] [review]

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?
Maybe we should just always make scroll layers fully visible? ie skip occlusion analysis of them in frame layer builder.
Comment on attachment 8526458 [details] [diff] [review]

I think we should do comment 4 instead of this.
Attachment #8526458 - Flags: review?(tnikkel)
Don't we need the correct clips applied for composition as well as for the occlusion check?
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.
Attached patch v2 (obsolete) — Splinter Review
Comment on attachment 8526458 [details] [diff] [review]

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.
Attachment #8526458 - Flags: review?(roc)
Comment #4 sounds right to me actually. What's the problem with it?
Attached patch patch (obsolete) — Splinter Review
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).
Attachment #8526458 - Attachment is obsolete: true
Attachment #8545382 - Attachment is obsolete: true
Attachment #8526458 - Flags: review?(roc)
Flags: needinfo?(dvander)
Attachment #8547931 - Flags: review?(roc)
Comment on attachment 8547931 [details] [diff] [review]

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!
Attachment #8547931 - Flags: review?(roc) → review-
Attached patch patchSplinter Review
w/ comments addressed
Attachment #8547931 - Attachment is obsolete: true
Attachment #8548565 - Flags: review?(roc)
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.