1166382 - Scrollbars (particularly the thumbs) are mispositioned when fullzoom is applied with APZ enabled

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

STR:
1. Load planet.mozilla.org in a desktop build that has APZ enabled.
2. Hit ctrl+shift+plus to apply some full zoom. Repeat a few times, observing the scrollbars

Expected:
Scroll thumb stays in the scroll track at all times and the scroll track stays at the edge of the content

Actual:
Scroll thumb sometimes is not in the scroll track. I've seen this on both OS X and Linux. On OS X if I enable the system setting to always-enable scrollbars I see the track also detach from the edge of the content sometimes. I assume Windows is also affected but didn't check explicitly.

Bug 1013377 may be related but I don't have a retina Mac so I can't confirm that. Keeping them separate for now.

Comment 1

[Botond Ballo [:botond]]

I can repro this on Linux. Annoys me quite a bit because I'm a heavy full-zoom user.

Taking.

Comment 2

[Botond Ballo [:botond]]

The thumb is correctly positioned at full zooms of 100%, 120%, 150%, and 200% - values which yield an integer FrameMetrics::mDevPixelsPerCSSPixel - and incorrectly positioned at full zooms of 110%, 133%, and 170%, which do not.

That leads me to believe the problem is related to the rounding error I described in bug 1155137 comment 11.

Comment 3

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #2)
> The thumb is correctly positioned at full zooms of 100%, 120%, 150%, and
> 200% - values which yield an integer FrameMetrics::mDevPixelsPerCSSPixel -

Er, I meant "an integer 'app units per dev pixel' scale".

Comment 4

[Botond Ballo [:botond]]

This comment [1] suggests that the canonical scale is the one that yields the rounded 'app units per dev pixel' scale. So, a full zoom of 133.0%, which would yield 45.11 app units per dev pixel, is adjusted to actually be 133.333...%, which yields 45 app units per dev pixel.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/src/nsDeviceContext.cpp?rev=ac4464790ec4#761

Comment 5

[Botond Ballo [:botond]]

Looks like nsDeviceContext adjusts the full zoom to a value that yields an integer app-units-per-dev-pixel, but nsPresContext fails to pick up the adjusted value. nsDocument::GetViewportInfo() queries nsPresContext for the full zoom, and makes calculations based on it that trickle down to TabChild::HandlePossibleViewportChange(), thus introducing an inaccurate scale into the system.

Comment 6

[Botond Ballo [:botond]]

Attachment: MozReview Request: bz://1166382/botond

/r/9497 - Bug 1166382 - Have nsPresContext pick up any adjustment nsDeviceContext makes to the full zoom. r=tn

Pull down this commit:

hg pull -r 2125d20b610828a4d1a9a32f3138d3698262ea1e https://reviewboard-hg.mozilla.org/gecko/

Comment 7

[Botond Ballo [:botond]]

This patch fixes the problem for me, but I'm quite out of my depth in this code.

Comment 8

[Timothy Nikkel (:tnikkel)]

https://reviewboard.mozilla.org/r/9497/#review8373

Ship It!

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8611518 [details]
MozReview Request: bz://1166382/botond

I don't get reviewboard. Much confused.

Comment 10

[Timothy Nikkel (:tnikkel)]

Wonder how we went this long without this showing up before.

Comment 11

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #10)
> Wonder how we went this long without this showing up before.

I think it's because nsPresContext::GetFullZoom() has very few callers compared to nsPresContext::AppUnitsPerDevPixel(), so most things work with the correct (adjusted) scale.

Comment 12

[Botond Ballo [:botond]]

This patch causes some zooming tests to fail [1]:

  browser/components/customizableui/test/browser_934951_zoom_in_toolbar.js
  browser/devtools/webide/test/test_zoom.html
  browser/base/content/test/general/browser_bug386835.js
  browser/base/content/test/general/browser_bug416661.js
  browser/base/content/test/general/browser_bug419612.js
  browser/base/content/test/general/browser_bug441778.js
  browser/base/content/test/general/browser_bug555224.js
  browser/base/content/test/general/browser_bug575830.js
  browser/base/content/test/general/browser_bug719271.js

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=805bb5841e7a

Comment 13

[Botond Ballo [:botond]]

The problem is that nsPresContext::GetFullZoom() is used as the value for the full zoom displayed in the UI. The current behaviour is to display the non-adjusted full zoom in the UI (for example, if you set the zoom to 110%, and we adjust it to 109% to give an integer number of app units per device pixel, we still display 110%); presumably we want to preserve that.

Comment 14

[Timothy Nikkel (:tnikkel)]

When the resulting zoom is relatively close to the requested zoom that seems reasonable. If the requested zoom is extreme and the resulting zoom is relatively far from it, then it seems less reasonable. The other option is to adjust the test expectations and if the UI wants to display 110% instead of 109% require it to keep track of that.

Comment 15

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #14)
> When the resulting zoom is relatively close to the requested zoom that seems
> reasonable. If the requested zoom is extreme and the resulting zoom is
> relatively far from it, then it seems less reasonable. The other option is
> to adjust the test expectations and if the UI wants to display 110% instead
> of 109% require it to keep track of that.

I think the adjustment we're talking about is for rounding only, not other things like clamping, so I don't think the adjusted zoom can be far from the requested zoom.

Comment 16

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1166382 - Use the adjusted full zoom in nsDocument::GetViewportInfo(). r=tn

Bug 1166382 - Use the adjusted full zoom in nsDocument::GetViewportInfo(). r=tn

Comment 17

[Botond Ballo [:botond]]

This is a more targeted fix which leaves PresContext::GetFullZoom() alone, and just changes nsDocument::GetViewportInfo() to use nsDeviceContext::GetFullZoom() to get the adjusted zoom instead.

I'll see how it fares on Try before posting for review.

Comment 18

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=090de9b25882

Comment 19

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #15)
> I think the adjustment we're talking about is for rounding only, not other
> things like clamping, so I don't think the adjusted zoom can be far from the
> requested zoom.

Yeah, but as we get closer to 1 app unit per dev pixel that difference gets larger, and when we exceed 1 app unit per dev pixel it blows up, an extreme case to be sure.

Comment 20

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #17)
> This is a more targeted fix which leaves PresContext::GetFullZoom() alone,
> and just changes nsDocument::GetViewportInfo() to use
> nsDeviceContext::GetFullZoom() to get the adjusted zoom instead.

I'd prefer if PresContext::GetFullZoom() returned the accurate zoom, and if we really need to babysit lazy callers we can have a PresContext::GetLastAppliedFullZoom() or something for places who want to know what they last called SetFullZoom with and took effect.

Comment 21

[Timothy Nikkel (:tnikkel)]

Since PresContext::GetFullZoom() is used by some layout things like layout/svg/nsSVGOuterSVGFrame.cpp

Comment 22

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #19)
> (In reply to Botond Ballo [:botond] from comment #15)
> > I think the adjustment we're talking about is for rounding only, not other
> > things like clamping, so I don't think the adjusted zoom can be far from the
> > requested zoom.
> 
> Yeah, but as we get closer to 1 app unit per dev pixel that difference gets
> larger, and when we exceed 1 app unit per dev pixel it blows up, an extreme
> case to be sure.

I think that's guarded against earlier in the pipeline. For example, the zoom in (+) button doesn't allow me to go beyond 300% full zoom.

Comment 23

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #20)
> (In reply to Botond Ballo [:botond] from comment #17)
> > This is a more targeted fix which leaves PresContext::GetFullZoom() alone,
> > and just changes nsDocument::GetViewportInfo() to use
> > nsDeviceContext::GetFullZoom() to get the adjusted zoom instead.
> 
> I'd prefer if PresContext::GetFullZoom() returned the accurate zoom, and if
> we really need to babysit lazy callers we can have a
> PresContext::GetLastAppliedFullZoom() or something for places who want to
> know what they last called SetFullZoom with and took effect.

I feel like that would be a more involved change, because we'd have to audit the various callers of PresContext::GetFullZoom() and decide for each whether we want the applied full zoom or the adjusted full zoom.

That's probably worth doing at some point, but I'm not convinced that it should block apz-nightly. The only call site that seems to be relevant to APZ is the one in GetViewportInfo() (because that feeds into a calculation used by APZ); if other call sites have issues, those issues are pre-existing.

Comment 24

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #18)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=090de9b25882

Looking green! I'd like to propose landing this, and filing a follow-up (which doesn't block apz-nightly) for the comprehensive audit of PresContext::GetFullZoom() callers.

Comment 25

[Botond Ballo [:botond]]

Comment on attachment 8613022 [details]
MozReview Request: Bug 1166382 - Use the adjusted full zoom in nsDocument::GetViewportInfo(). r=tn

Bug 1166382 - Use the adjusted full zoom in nsDocument::GetViewportInfo(). r=tn

Comment 27

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8613022 [details]
MozReview Request: Bug 1166382 - Use the adjusted full zoom in nsDocument::GetViewportInfo(). r=tn

https://reviewboard.mozilla.org/r/9497/#review8523

Ship It!

Comment 28

[Timothy Nikkel (:tnikkel)]

We need a followup bug to fix this though. Of the ~10 callers of GetFullZoom, 9 obviously wanted the proper zoom.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8095c9256ae2

Comment 30

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #28)
> We need a followup bug to fix this though. Of the ~10 callers of
> GetFullZoom, 9 obviously wanted the proper zoom.

Filed bug 1170317.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8095c9256ae2