Closed Bug 813041 Opened 7 years ago Closed 6 years ago

always layerize the scroll indicator

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
blocking-b2g koi+
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: gal, Assigned: mattwoodrow)

References

Details

Attachments

(4 files, 4 obsolete files)

Right now we repaint the scroll indicator every time we move it. Its just a simple nsDisplayBackground so thats not too bad. However, we also have to repaint the area that becomes visible underneith it, which sucks and can be slow. We should put the indicator into its own layer. Its smallish anyway.
What do you guys think?
  // We put scrollbars in their own layers when this is the root scroll
  // frame and we are a toplevel content document. In this situation, the
  // scrollbar(s) would normally be assigned their own layer anyway, since
  // they're not scrolled with the rest of the document. But when both
  // scrollbars are visible, the layer's visible rectangle would be the size
  // of the viewport, so most layer implementations would create a layer buffer
  // that's much larger than necessary. Creating independent layers for each
  // scrollbar works around the problem.
  bool createLayersForScrollbars = mIsRoot &&
    mOuter->PresContext()->IsRootContentDocument();

I don't think this is kicking in, otherwise we shouldn't see invalidation.
Actually, its kicking in, and we invalidate anyway, which is a bug. So this is invalid.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Actually, not invalid after all. We layerize, but we still invalidate. Instead, we want to detect that we aren't actually changing what we paint, just moving the scrollbox thumb.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Assignee: nobody → matt.woodrow
Apparently drawing the scrollbar costs b2g around 10fps when scrolling, so maybe this is worthwhile.

This completely breaks scrollbars on mac because the SliderFrame/thumb frame that we create don't actually paint anything and the entire scrollbar (thumb and all) is painted by ScrollbarFrame.
The 'thumb' display items that are created do nothing except trick DLBI into invalidating the correct pixels. Shifting these items into their own layer stops this, so we never actually repaint the scrollbar.

I think this should work on b2g though, assuming we do sane things for painting the scrollbar there.

Does this look like a reasonable approach roc? Obviously needs a fair bit of work, the perspective change to force an active layer is my favourite bit :)
Attachment #683459 - Flags: feedback?(roc)
(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> This completely breaks scrollbars on mac because the SliderFrame/thumb frame
> that we create don't actually paint anything and the entire scrollbar (thumb
> and all) is painted by ScrollbarFrame.

So nsNativeThemeCocoa::WidgetStateChanged should probably return true if aWidgetType is NS_THEME_SCROLLBAR or NS_THEME_SCROLLBAR_SMALL and aAttribute is curpos, minpos, maxpos or pageincrement. That would also have been the right fix to bug 385058 (which was my first patch ever).
Why are we putting the whole slider into a layer instead of just the thumb?
We don't? The nsDisplayTransform is only wrapping the children of the nsSliderFrame.
Thanks Markus.

Looks like I inadvertently reverted bug 385058 with DLBI. I haven't seen any regressions filed, so I guess we no longer use a theme that paints outside the bounds of the thumb frame.

Your suggestion works, but now we're back to painting the entire scrollbar instead of only the thumb area. Maybe that's not an issue, since it's what we were doing pre-DLBI too. Should only affect OSX.

It would be nice if we could paint the thumb separately to the scrollbar, or have WidgetStateChanged return a rect to invalidate. Probably outside the scope of this bug though.
Comment on attachment 683459 [details] [diff] [review]
WIP: Shift the scrollbar thumb using an active transform

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

::: layout/xul/base/src/nsSliderFrame.h
@@ +129,5 @@
> +      temp = gfx3DMatrix::Translation(NSAppUnitsToFloatPixels(mSliderOffset, aAppUnitsPerPixel), 0, 0);
> +    } else {
> +      temp = gfx3DMatrix::Translation(0, NSAppUnitsToFloatPixels(mSliderOffset, aAppUnitsPerPixel), 0);
> +    }
> +    temp._43 = 1;

Why is this line needed?
Attachment #683459 - Flags: feedback?(roc) → feedback+
With the attached patch we are still invalidating the scroll bar thumb every time it moves.
Sorry, forgot to upload the latest patch. This one works (I hope).

Still failing some tests on try though currently.
Attachment #683459 - Attachment is obsolete: true
Attached patch scrollbar wip rebased. (obsolete) — Splinter Review
I just rebased the WIP to play with it since I would like to turn the scrollbars on again for b2g (bug 876741). Works well for me.
Same rebased wip, but this one compiles...
Attachment #819742 - Attachment is obsolete: true
No longer blocks: 876741
So... it does block bug 876741?  Since that one is koi+, this one should be as well.  Can somebody clarify the dependency?
blocking-b2g: --- → koi?
(In reply to Milan Sreckovic [:milan] from comment #15)
> So... it does block bug 876741?  Since that one is koi+, this one should be
> as well.  Can somebody clarify the dependency?
(In reply to Milan Sreckovic [:milan] from comment #15)
> So... it does block bug 876741?  Since that one is koi+, this one should be
> as well.  Can somebody clarify the dependency?

Please see comment 5.

"Apparently drawing the scrollbar costs b2g around 10fps when scrolling, so maybe this is worthwhile."
Blocking a blocker
blocking-b2g: koi? → koi+
NI :mattwoodrow to give an eta on fixing this and set a 1.2 target milestone as this blocks 876741
Flags: needinfo?(matt.woodrow)
I haven't looked at this since 2012, I assumed Vivien was working on this and just hadn't changed the assignee.

FWIW, I don't think the existing prototype is a great way to go about this, doing something similar to bug 876321 would be easier.
Flags: needinfo?(matt.woodrow)
Can you let me know if this works for b2g?

I know of at least two issues with this patch:

1) It still triggers invalidation for desktop since the scrollbar thumb moves by subpixel amounts. We don't retain content for subpixel moves since it tends to look bad, but we ignore this for mobile (for performance reasons).

2) It will break scrollbars for the old (10.6?) style OSX scrollbars, though maybe only if 1 is fixed. These scrollbars render the entire thing (track, thumb etc) as a single display item, and the thumb display item is just a placeholder to trigger invalidation. If we're not invalidating it, then nothing will trigger the real item behind it to be repainted.
Attachment #827005 - Flags: feedback?(21)
Comment on attachment 827005 [details] [diff] [review]
Push the scrollbar into it's own ThebesLayer

I'm not working on this patch fwiw, sorry for the miscommunication issue.
About panning it seems to not regress too with this patch.
Attachment #827005 - Flags: feedback?(21) → feedback+
Comment on attachment 827005 [details] [diff] [review]
Push the scrollbar into it's own ThebesLayer

Roc, how would you feel about putting this inside a mobile/b2g only #ifdef and landing it as is?

Getting it to work properly on desktop is fairly tricky, and would require runtime checks to check what type of scrollbars are enabled. I don't think we want that overhead in GetActiveScrolledRootFor.

Besides, it's unlikely to ever matter performance wise for desktop.
Attachment #827005 - Flags: review?(roc)
Comment on attachment 827005 [details] [diff] [review]
Push the scrollbar into it's own ThebesLayer

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

Replace the do_QueryFrame check below with a type check using the cached result of GetType(), so we don't add any virtual calls here.
Attachment #827005 - Flags: review?(roc) → review-
Attachment #827005 - Attachment is obsolete: true
Attachment #828510 - Flags: review?(roc)
Comment on attachment 828510 [details] [diff] [review]
Push the scrollbar into it's own ThebesLayer v2

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

Patch is unchanged?
Attachment #828510 - Flags: review?(roc) → review-
Now with more qref.
Attachment #828510 - Attachment is obsolete: true
Attachment #828520 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/6c40724f1a0e
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment on attachment 828520 [details] [diff] [review]
Push the scrollbar into it's own ThebesLayer v3

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

::: layout/base/nsLayoutUtils.cpp
@@ +1265,5 @@
> +    // Treat the slider thumb as being as an active scrolled root
> +    // on mobile so that it can move without repainting.
> +    if (parentType == nsGkAtoms::sliderFrame)
> +      break;
> +#endif

Hmm ... actually this needs to apply to b2g too.

I suggest we add a method to the slider frame to check whether it should be treated as an active scrolled root, and that method should call LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars).
Holding off on the b2g26 uplift pending comment 29.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> Comment on attachment 828520 [details] [diff] [review]
> Push the scrollbar into it's own ThebesLayer v3
> 
> Review of attachment 828520 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsLayoutUtils.cpp
> @@ +1265,5 @@
> > +    // Treat the slider thumb as being as an active scrolled root
> > +    // on mobile so that it can move without repainting.
> > +    if (parentType == nsGkAtoms::sliderFrame)
> > +      break;
> > +#endif
> 
> Hmm ... actually this needs to apply to b2g too.
> 
> I suggest we add a method to the slider frame to check whether it should be
> treated as an active scrolled root, and that method should call
> LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars).

ANDROID should be defined on b2g too I believe.

As for getting it to work on desktop, I think we just need to follow Markus' suggestion from comment 6. And the change the nsSliderFrame code to snap the thumb positions to integer pixel positions.

The latter part seemed likely to break reftests though, and I doubt it's much of a performance win for desktop.

Assuming that I'm right about the #define, then I think we should just stick with that and uplift it.

We can file a follow-up for desktop if you think it's worthwhile. Thoughts?
Flags: needinfo?(roc)
(In reply to Matt Woodrow (:mattwoodrow) from comment #31)
> Assuming that I'm right about the #define, then I think we should just stick
> with that and uplift it.
> 
> We can file a follow-up for desktop if you think it's worthwhile. Thoughts?

OK.
Flags: needinfo?(roc)
Not entirely sure how to resolve this for b2g26. Please post a branch-specific patch :)
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(matt.woodrow)
NI RyanVM to help with the landing here ?
Flags: needinfo?(ryanvm)
Sorry about that, there was a flaw in my bug queries that caused me to miss this. I've fixed it so it shouldn't happen again.

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/c60fd54b599b
Flags: needinfo?(ryanvm)
Depends on: 1009679
You need to log in before you can comment on or make changes to this bug.