Classic scrollbars don't draw if they overlap a scrollframe

RESOLVED FIXED in mozilla38

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

unspecified
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Posted 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.
Posted patch bug1100756-clip.patch (obsolete) — Splinter Review
Attachment #8526458 - Flags: review?(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 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?
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]
bug1100756-clip.patch

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.
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.
Attachment #8526458 - Flags: review?(roc)
Comment #4 sounds right to me actually. What's the problem with it?
Posted 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]
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!
Attachment #8547931 - Flags: review?(roc) → review-
Posted patch patchSplinter Review
w/ comments addressed
Attachment #8547931 - Attachment is obsolete: true
Attachment #8548565 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/650de89062f6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.