Overlay scrollbars misplaced when content is zoomed

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: kanru, Assigned: kats)

Tracking

Trunk
mozilla35
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.0 unaffected, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

()

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Description

5 years ago
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.
Component: Panning and Zooming → Layout
Posted image 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.
Summary: [APZ] When zooming out, the scroll bar is still at the old position → Overlay scrollbars misplaced when content is zoomed
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.
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).
Okay, so maybe they are just in the wrong place before that code tries to place them.
[Blocking Requested - why for this release]:
This looks really bad if you zoom out on non-mobile pages in the browser.
blocking-b2g: --- → 2.1?
See Also: → 1056423
: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.
Flags: needinfo?(bugs)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
Flags: needinfo?(bugs) → needinfo?(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.
Flags: needinfo?(tnikkel)
Bug 1038178 is for example one change that is using the root frame scrollbars.
See Also: → 1038178
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))
Milan - is this something we could explore fixing for 2.1?
Flags: needinfo?(milan)
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.
Absolutely.  Let's take the uplift route.
blocking-b2g: 2.1? → backlog
Component: Layout → Panning and Zooming
Flags: needinfo?(milan)
Assignee: nobody → tnikkel
Blocks: 1033468
See Also: 1056423
Posted patch wip (obsolete) — Splinter Review
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.
Posted patch wip (obsolete) — Splinter Review
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Assignee: tnikkel → bugmail.mozilla
See Also: 1036264
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.
Attachment #8496502 - Attachment is obsolete: true
Attachment #8497605 - Flags: review?(botond)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
Flags: needinfo?(tnikkel)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Flags: needinfo?(tnikkel) → needinfo?(bugmail.mozilla)
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.
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?
Flags: needinfo?(bugmail.mozilla) → needinfo?(tnikkel)
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.
Attachment #8497598 - Flags: review?(botond) → review+
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.
Attachment #8497605 - Flags: review?(botond) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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
Depends on: 1076447
Filed bug 1076447 for the clipping issue.
Flags: needinfo?(tnikkel)
https://hg.mozilla.org/mozilla-central/rev/1c4d997e2260
https://hg.mozilla.org/mozilla-central/rev/0b4ff0427b86
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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
Attachment #8497598 - Flags: approval-mozilla-aurora?
Attachment #8497605 - Flags: approval-mozilla-aurora?
Blocks: 1077402
No longer blocks: 1077402
Depends on: 1077402
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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..
Flags: needinfo?(tchung)
Flags: needinfo?(bugmail.mozilla)
Also, do we have any reftests for this new feature ? If so, how this was missed?
(In reply to bhavana bajaj [:bajaj] from comment #30)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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..
Flags: needinfo?(tchung)
(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.
Flags: needinfo?(bugmail.mozilla)
[Blocking Requested - why for this release]:

If we want bug 995519 uplifted for 2.1, we need this bug fixed as well.
blocking-b2g: backlog → 2.1?
(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+
Attachment #8497598 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8497605 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Part 2 needs some rebasing around bug 1074944 (or an approval request).
Flags: needinfo?(bugmail.mozilla)
I'll rebase and uplift once 1077402 gets aurora+ as it would be good to land these together (or have that one land first).
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 8495328 [details] [diff] [review]
wip

Moved to bug 1078316
Attachment #8495328 - Attachment is obsolete: true
blocking-b2g: 2.1? → ---
Depends on: 1165536
You need to log in before you can comment on or make changes to this bug.