1043859 - Overlay scrollbars misplaced when content is zoomed

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

Find in bug 1036264

https://bugzilla.mozilla.org/attachment.cgi?id=8461385

While testing my test page on the phone, I found that when zooming out, the scroll bar is still at the old position. When zooming in, the scroll bar is not visible.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Screenshot

It's hard to tell from the screenshot, but it looks like the scrollbars are always at the CSS viewport size rather than the scroll-position-clamping-scroll-port-size which would be more correct. For non-root scrollframes this doesn't make a difference because they are never zoomable. For root scrollframes the two can diverge but until recently we didn't have scrollbars enabled on those.

Comment 2

[Timothy Nikkel (:tnikkel)]

Layout can position the scrollbars differently but that won't help during a pinch zoom. I think APZC is going to have to deal with some positioning of scrollbars.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

I believe that code already exists and is in place. It's used during async scroll as well. I don't think I've explicitly tested the pinch zoom case but as far as I know it should Just Work (TM).

Comment 4

[Timothy Nikkel (:tnikkel)]

Okay, so maybe they are just in the wrong place before that code tries to place them.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

[Blocking Requested - why for this release]:
This looks really bad if you zoom out on non-mobile pages in the browser.

Comment 6

[bhavana bajaj [:bajaj]]

:jet can you please help triage this for 2.1? Not sure if you need any QA input here but feel free to add qawanted if you need any additional qa help here.

Comment 7

[Jet Villegas (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)
> For root scrollframes the two can diverge but until recently we
> didn't have scrollbars enabled on those.

Should there be scrollbars at all? Is that the regression here? Assuming we want the scrollbars, which bug enabled that feature?

Comment 8

[Timothy Nikkel (:tnikkel)]

Bug 995519 added these scrollbars, it was by request that we landed the patch, so they are definitely wanted at least some of the time.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Bug 1038178 is for example one change that is using the root frame scrollbars.

Comment 10

[Chris Lord [:cwiiis]]

We definitely want the scrollbars, otherwise we can't show scrollbars in the browser (which I do believe we want to do, and is certainly what we do on all non-b2g shipping platforms (and b2g, now that this was fixed))

Comment 11

[Kevin Grandon :kgrandon]

Milan - is this something we could explore fixing for 2.1?

Comment 12

[Kartikaya Gupta (email:[e-mail])]

I definitely think this is something we should fix in 2.1 (hence my nomination for 2.1+). Timothy is planning to look into it once he's done with the current imagelib reviews.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Absolutely.  Let's take the uplift route.

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: wip

Initial patch, still many problems to fix, but it should be enough to work out some issues with the transforms that async composition manager applies to scrollbars.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: wip

After much wrangling with transform matrices I discovered that this compositor-side patch by itself fixes the positioning problem of the scrollbars. There's still a couple of issues - one is that they don't play well with the dynamic rocketbar. When the rocketbar is expanded the horizontal scrollbar goes off-screen. The other is that when you scroll to the bottom of a page with some zoom applied the scrollbar seems to get clipped at the bottom, and I'm not sure why.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> When the rocketbar is expanded the
> horizontal scrollbar goes off-screen.

This happens to fixed-position elements too so I'm not gonna bother worrying about it for the scrollbars. If we want to address this we will have to do it at a higher level than just scrollbars.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Expose the APZ overscroll separately

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Fix descendant-scrollbar transforms

Assume the layer with the scrollbar tag is layer L and the layer with the APZ is R; L is a descendant of R. If R has a resolution of z, then the effective transform on L is as follows:

prescale(L) * shadowtransform(L) * postscale(L) * prescale(M) * ... postscale(Q) * prescale(R) * shadowtransform(R) * postscale(R)

For layers M through Q the prescale will be 1/z, the shadowtransform will be z, and the postscale will be 1, so those all cancel out and we can ignore them. If we break down the shadowtransform into inv-prescale * transform * asynctransform * inv-postscale, we then get:

prescale(L) * inv-prescale(L) * transform(L) * asynctransform(L) * inv-postscale(L) * postscale(L) * prescale(R) * inv-prescale(R) * transform(R) * asynctransform(R) * inv-postscale(R) * postscale(R)

which simplifies to:

transform(L) * transform(R) * asynctransform(R).

Therefore, to remove the effects of only asynctransform(R) on L, we need to post-multiply transform(L) by transform(R) * inv-asynctransform(R) * inv-transform(R). That is what this patch does.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> The other is that when you scroll to
> the bottom of a page with some zoom applied the scrollbar seems to get
> clipped at the bottom, and I'm not sure why.

This "clipping" I believe happens in layout code; the visible rect of the scrollbar layer gets truncated as you approach the bottom of the page while zoomed in. I believe this happens because the scrollbars are laid out to the composition bounds, but the visible rect is not. I suspect that the visible rect is computed using the scroll-position-clamping-scroll-port-size, because that's the thing that shrinks as you zoom in and would result in the visible rect getting clipped.

In order to get the scrollbars to render at the correct "fatness" I think we do want to leave the scrollbars as being laid out to the composition bounds. Is it possible to just change the visible-rect computation for the scrollbar layers in layout so that they don't get clipped?

Comment 20

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> > The other is that when you scroll to
> > the bottom of a page with some zoom applied the scrollbar seems to get
> > clipped at the bottom, and I'm not sure why.
> 
> This "clipping" I believe happens in layout code; the visible rect of the
> scrollbar layer gets truncated as you approach the bottom of the page while
> zoomed in. I believe this happens because the scrollbars are laid out to the
> composition bounds, but the visible rect is not. I suspect that the visible
> rect is computed using the scroll-position-clamping-scroll-port-size,
> because that's the thing that shrinks as you zoom in and would result in the
> visible rect getting clipped.

I'm confused about what you are asking for because isn't the scroll-position-clamping-scroll-port-size the same thing as the composition bounds? We only set the scroll-position-clamping-scroll-port-size to CalculateCompositedSizeInCssPixels(), which is the composition size in CSS pixels.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Hm, I might be wrong. In my head I was thinking that the scrollbars were laid out to the composition bounds in ParentLayerPixels, and so their position didn't change (relative to the content they're scrolling) as you zoom. However the clip rect does.

At any rate I can try to get display list dumps of this happening to better understand the problem. Leaving needinfo on me for now.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Attachment: Browser process display list dumps

Here's a set of display list dumps from the browser. If you run |grep "(0xb3a44440)"| on it, you'll see the client-side layer dumps for the layer where the visible rect height starts off at 109 and then changes to 94 and 52 as it gets truncated. tn, can you tell why that visible rect is getting shortened?

Comment 23

[Botond Ballo [:botond]]

Comment on attachment 8497598 [details] [diff] [review]
Part 1 - Expose the APZ overscroll separately

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

Yay for value semantics and methods that do one thing.

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8497605 [details] [diff] [review]
Part 2 - Fix descendant-scrollbar transforms

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

Nice find!

Technically, if any of the layers M ... Q have a non-async transform (other than a scale:z) for some reason, we'd need to apply those too, but I think that shouldn't happen in cases where L is the scrollbar's layer and R is the layer being scrolled.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +740,5 @@
> +    Matrix4x4 contentTransform = aContent.GetTransform();
> +    Matrix4x4 contentUntransform = contentTransform;
> +    contentUntransform.Invert();
> +
> +    Matrix4x4 compensation = contentTransform * asyncUntransform * contentUntransform;

Let's add a comment saying that since the async transform that we want to unapply applies on top of the content's (regular) transform, we need to apply the (regular) transform to be in a space where we can unapply the async transform.

Comment 25

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #21)
> Hm, I might be wrong. In my head I was thinking that the scrollbars were
> laid out to the composition bounds in ParentLayerPixels, and so their
> position didn't change (relative to the content they're scrolling) as you
> zoom. However the clip rect does.

Like all layout, layout of scrollbars happens in appunits.

Comment 26

[Kartikaya Gupta (email:[e-mail])]

I landed the two patches so that at least they don't bitrot, and will spin off another bug for the clipping issue. We can continue the discussion there.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c4d997e2260
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b4ff0427b86

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Filed bug 1076447 for the clipping issue.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1c4d997e2260
https://hg.mozilla.org/mozilla-central/rev/0b4ff0427b86

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8497598 [details] [diff] [review]
Part 1 - Expose the APZ overscroll separately

Approval Request Comment (for both patches)
[Feature/regressing bug #]: bug 995519
[User impact if declined]: in the B2G browser the scrollbars appear in the wrong position after changing the zoom
[Describe test coverage new/current, TBPL]: local manual testing
[Risks and why]: not a large risk in the uplift, but this bug by itself doesn't fix all the problems with the scrollbar. I filed bug 1076447 for the remaining issue. Another alternative is to simply disable the root-frame scrollbars in 2.1, which would be preferable to having them enabled but wrong. They were disabled in 2.0 and earlier.
[String/UUID change made/needed]: none

Comment 30

[bhavana bajaj [:bajaj]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #29)
> Comment on attachment 8497598 [details] [diff] [review]
> Part 1 - Expose the APZ overscroll separately
> 
> Approval Request Comment (for both patches)
> [Feature/regressing bug #]: bug 995519
> [User impact if declined]: in the B2G browser the scrollbars appear in the
> wrong position after changing the zoom
> [Describe test coverage new/current, TBPL]: local manual testing
> [Risks and why]: not a large risk in the uplift, but this bug by itself
> doesn't fix all the problems with the scrollbar. I filed bug 1076447 for the
> remaining issue. Another alternative is to simply disable the root-frame
> scrollbars in 2.1, which would be preferable to having them enabled but
> wrong. They were disabled in 2.0 and earlier.
> [String/UUID change made/needed]: none

That's for the input of risk analysis here kats. A couple of questions/concerns :

* I am leaning towards the alternative you proposed on disabling the root-frame scrollbars in 2.1 , given the fallout you've identified and an unknown risk here as QA does not seem to have tested this feature and the timeline..
* Doing the backout gets us the status quo with 2.0
* How risky and easy is the backout ? Is it behind a pref ? If not, can we create one ? 
* I am trying to get a sense from QA on testing this feature and if their preliminary testing looks good, I am fine to take this uplift for now. But if we keep discovering more regressions over the next days, we'd want this preffed-off immediately.
* If there is no easy way to pref this out later, I'd rather backout this out sooner than later before it gets more complicated with a lot of uplifts..

Comment 31

[bhavana bajaj [:bajaj]]

Also, do we have any reftests for this new feature ? If so, how this was missed?

Comment 32

[Tony Chung [:tchung]]

(In reply to bhavana bajaj [:bajaj] from comment #30)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #29)
> > Comment on attachment 8497598 [details] [diff] [review]
> > Part 1 - Expose the APZ overscroll separately
> > 
> > Approval Request Comment (for both patches)
> > [Feature/regressing bug #]: bug 995519
> > [User impact if declined]: in the B2G browser the scrollbars appear in the
> > wrong position after changing the zoom
> > [Describe test coverage new/current, TBPL]: local manual testing
> > [Risks and why]: not a large risk in the uplift, but this bug by itself
> > doesn't fix all the problems with the scrollbar. I filed bug 1076447 for the
> > remaining issue. Another alternative is to simply disable the root-frame
> > scrollbars in 2.1, which would be preferable to having them enabled but
> > wrong. They were disabled in 2.0 and earlier.
> > [String/UUID change made/needed]: none
> 
> That's for the input of risk analysis here kats. A couple of
> questions/concerns :
> 
> * I am leaning towards the alternative you proposed on disabling the
> root-frame scrollbars in 2.1 , given the fallout you've identified and an
> unknown risk here as QA does not seem to have tested this feature and the
> timeline..
> * Doing the backout gets us the status quo with 2.0
> * How risky and easy is the backout ? Is it behind a pref ? If not, can we
> create one ? 
> * I am trying to get a sense from QA on testing this feature and if their
> preliminary testing looks good, I am fine to take this uplift for now. But
> if we keep discovering more regressions over the next days, we'd want this
> preffed-off immediately.

i've asked in an offline thread for more information, before committing QA to this late testing work.

> * If there is no easy way to pref this out later, I'd rather backout this
> out sooner than later before it gets more complicated with a lot of uplifts..

Comment 33

[Kartikaya Gupta (email:[e-mail])]

(In reply to bhavana bajaj [:bajaj] from comment #30)
> That's for the input of risk analysis here kats. A couple of
> questions/concerns :
> 
> * I am leaning towards the alternative you proposed on disabling the
> root-frame scrollbars in 2.1 , given the fallout you've identified and an
> unknown risk here as QA does not seem to have tested this feature and the
> timeline..
> * Doing the backout gets us the status quo with 2.0
> * How risky and easy is the backout ? Is it behind a pref ? If not, can we
> create one ? 
> * I am trying to get a sense from QA on testing this feature and if their
> preliminary testing looks good, I am fine to take this uplift for now. But
> if we keep discovering more regressions over the next days, we'd want this
> preffed-off immediately.
> * If there is no easy way to pref this out later, I'd rather backout this
> out sooner than later before it gets more complicated with a lot of uplifts..

(In reply to bhavana bajaj [:bajaj] from comment #31)
> Also, do we have any reftests for this new feature ? If so, how this was
> missed?

We mostly covered this stuff on the email thread, re-summarizing here for posterity:
- Disabling the scrollbars is a one-line patch, should be pretty safe. Can create a pref if needed but it probably isn't worth it. The patch to disable should work even with this uplifted.
- Reftests for scrollbar behaviour are very hard to make cross-platform, particularly for overlay scrollbars since they fade out. They are very prone to intermittent failures/fuzzing and a lot of reftests disable them entirely.

Comment 34

[Tony Chung [:tchung]]

[Blocking Requested - why for this release]:

If we want bug 995519 uplifted for 2.1, we need this bug fixed as well.

Comment 35

[Kartikaya Gupta (email:[e-mail])]

(In reply to Tony Chung [:tchung] from comment #34)
> If we want bug 995519 uplifted for 2.1

Bug 995519 is already in 2.1. I'm not sure that making this bug 2.1+ has any additional value over just aurora+

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Part 2 needs some rebasing around bug 1074944 (or an approval request).

Comment 37

[Kartikaya Gupta (email:[e-mail])]

I'll rebase and uplift once 1077402 gets aurora+ as it would be good to land these together (or have that one land first).

Comment 38

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8495328 [details] [diff] [review]
wip

Moved to bug 1078316

Comment 39

[Kartikaya Gupta (email:[e-mail])]

Try push for the rebased patches: https://tbpl.mozilla.org/?tree=Try&rev=4b007739646c

Landed on aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/398ee670cb58
https://hg.mozilla.org/releases/mozilla-aurora/rev/4596fc5abb41