Closed Bug 977861 Opened 6 years ago Closed 6 years ago

We always repaint the scrollbar thumb on B2G

Categories

(Core :: Layout, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

Details

Attachments

(2 files, 1 obsolete file)

This causes needless sadness.
Interesting, I assumed that was already disabled for b2g. Maybe it should be?
This also does the trick. I'm not sure what the best way to do this is.
Comment on attachment 8384755 [details] [diff] [review]
Only move the scroll thumb to integer number of pixels

Markus, you seem to have touched this code last. Any thoughts on this basic idea?
Attachment #8384755 - Flags: feedback?(mstange)
It's worth noting that this patch seems to cause the scrollbar to jiggle a little bit. Presumably this is because of rounding canceling in different directions in different places.
We should probably do the snapping in display list code like we do for everything else. That will also make the snapping take into account any scale from layout device pixels to layer pixels.
(In reply to Timothy Nikkel (:tn) from comment #6)
> We should probably do the snapping in display list code like we do for
> everything else. That will also make the snapping take into account any
> scale from layout device pixels to layer pixels.

What would that look like?
So it looks like scrollbars are like so on b2g
OwnLayer 44c9a3c8(ScrollbarFrame(scrollbar)(-1)) bounds(18720,9104,360,3900) visible(0,0,0,0) componentAlpha(0,0,0,0) clip(0,0,19200,27600)  layer=44c0f000
  WrapList 44c9a3c8(ScrollbarFrame(scrollbar)(-1)) bounds(18720,9104,360,3900) visible(0,0,0,0) componentAlpha(0,0,0,0) clip() 
    BackgroundColor 44c9a838(ButtonBoxFrame(thumb)(0)) bounds(18720,9104,360,3900) visible(0,0,0,0) componentAlpha(0,0,0,0) clip(0,0,19200,27600)  (rgba 0,0,0,102) layer=42eecc90
    Border 44c9a838(ButtonBoxFrame(thumb)(0)) bounds(18720,9104,360,3900) visible(0,0,0,0) componentAlpha(0,0,0,0) clip(0,0,19200,27600)  layer=42eecc90

The wraplist gets flattened away. Background color and borders both snap so either the wraplist is causing the problem (it's GetBounds specifically says it doesn't snap) or the ownlayer item is (it's a subclass of wrapitem and it doesn't override GetBounds).

So I would say try overriding GetBounds on nsDisplayOwnLayer, if the nsDisplayOwnLayer has VERTICAL_SCROLLBAR or HORIZONTAL_SCROLLBAR flag then walk all of mList calling GetBounds and checking the aSnap outparam, if it's true for all of them then set ourparam aSnap to true and return nsDisplayWrapList::GetBounds, otherwise set it to false. Or just make nsDisplayOwnLayer::GetBounds return aSnap as true for all scrollbars (as identified by the flags) to make a quick check that this is the right path.
(In reply to Timothy Nikkel (:tn) from comment #8)

We're using the coordinates directly from the frame here:
http://dxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#1524

Won't snapping the the displaylist item have no effect?
I think that's the problem. We snap the display items, but we don't take that into account when determining the top-left of the ThebesLayer.
Comment on attachment 8384755 [details] [diff] [review]
Only move the scroll thumb to integer number of pixels

I'd use something like
nscoord appUnitsPerPixel = PresContext()->AppUnitsPerDevPixel();
thumbFrame->SetRect(newThumbRect.ToNearestPixels(appUnitsPerPixel).ToAppUnits(appUnitsPerPixel));

But fixing the issue in comment 9 / comment 10 is probably the cleaner approach.
Attachment #8384755 - Flags: feedback?(mstange)
(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> I think that's the problem. We snap the display items, but we don't take
> that into account when determining the top-left of the ThebesLayer.

How do we fix it?
That's tough :)

Even if all the items within a ThebesLayer have snapping enabled, changes to the animated geometry root position might make items that previously snapped to the same pixel no longer do so.

Consider the following example (Y axis only):

>Two display items, one (belonging to the active geometry root frame) at 0.5px, the other at 1px.
>
>We compute a thebes layer transform of 0px, active geometry root starts at 0.5px into the ThebesLayer.
>Both items snap to being drawn starting at 1px.
>
>Then we scroll by 0.5px, so that our two items are at 1px and 1.5px respectively.
>
>Now the thebes layer transform is 1px, and the active geometry root starts at 0px. The first item snaps to being drawn at 0px >into the layer, the second still snaps to 1px.

I think what we could do is:

If we detect a subpixel change in the active geometry root, instead of invalidating the layer immediately, mark it for deferred invalidation (during ContainerState::Finish maybe).

Once we start adding items to the layer, we can check if all items in the layer are snapped, and will snap to the same relative places as last time. If that is true then we can skip the invalidation.

For computing if they will snap the same, a simple method (but not exhaustive) method might be to check if they all have the same coordinates in the axis that changed. Looks like that should be true for this case at least (both items have a y position of 9104).

Adidng ni?roc to see if he has any ideas.
Flags: needinfo?(roc)
What is the animated geometry root frame here? The scrollbar thumb itself? What causes that?

In this case, if we don't snap the display items for the scrollbar thumb, that should make the problem go away, right? Or really what should happen is that they snap relative to their animated geometry root. Maybe we can do that?
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> What is the animated geometry root frame here? The scrollbar thumb itself?
> What causes that?

Yes. The code for choosing an animated geometry root checks for slider frames (mobile only).

> 
> In this case, if we don't snap the display items for the scrollbar thumb,
> that should make the problem go away, right? Or really what should happen is
> that they snap relative to their animated geometry root. Maybe we can do
> that?

The problem is that when we recycle the ThebesLayer, we detect that the active geometry root (the slider frame) has moved relative to the ViewportFrame by something that isn't a whole number of pixels. We then forcibly invalidate the entire layer.

This all happens before we actually process any items, so changing snapping on them won't have any effect.

How do we snap relative to the active geometry root? We only know how to snap to pixels, and the active geometry root isn't necessarily aligned to a pixel boundary. We also don't want a non-integer translation on the ThebesLayer.
See Also: → 980647
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Flags: needinfo?(roc)
(In reply to Matt Woodrow (:mattwoodrow) from comment #15)
> How do we snap relative to the active geometry root? We only know how to
> snap to pixels, and the active geometry root isn't necessarily aligned to a
> pixel boundary. We also don't want a non-integer translation on the
> ThebesLayer.

Can we just allow that in this case? I.e. hacked for the slider?
Flags: needinfo?(roc)
I guess we can, resampling the scrollbar when compositing might look bad though.

Jeff, does this work?
(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> Created attachment 8388914 [details] [diff] [review]
> Allow non-integer transforms for the scrollbar
> 
> I guess we can, resampling the scrollbar when compositing might look bad
> though.
> 
> Jeff, does this work?

This patch does not work. We still go down into invalidating the entire layer.
The solution for fixing this properly is not clear and could be a lot of work. Matt Woodrow agreed that this is probably the right thing to do for now.

This is b2g only because it was causing reftest failures on OS X that I do not understand and is pretty important for performance.
Attachment #8384755 - Attachment is obsolete: true
Attachment #8391231 - Flags: review?(mstange)
Attachment #8391231 - Flags: review?(mstange) → review+
https://hg.mozilla.org/mozilla-central/rev/96c7ae85ff76
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.