Alice0775 White Reporter
	Description • 9 years ago

Build Identifier:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e735b337a1e2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0 ID:20150523072946

When I test Bug 1164406, I noticed the glitch.
I tested apz on latest m-i tinderbox build which include patch of Bug 1164406.

Reproducible: always

Steps To Reproduce:
1. Enable APZ
   user_pref("layers.async-pan-zoom.enabled", true);
2. Open any page
        e.g.  http://www.snapdeal.com/product/hp-15r205tu-notebook-k8u05pa-5th/683139390240 
              https://support.mozilla.org/en-US/kb/get-started-firefox-overview-main-features

3. Wait loading completion
4. Resize Browser Window
   --- Bug happens
5. Attempt to scroll page by wheel etc

Actual Results:
no scrollbar are shown
some fixed position elements are broken.

Especially about www.snapdeal.com
Reload(f5) does not fix
I must close the glitch tab

	Alice0775 White Reporter
	Comment 1 • 9 years ago

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=12cf9fdc778f&tochange=256f3225ca4e

Regressed by: Bug 1164406

	Alice0775 White Reporter
	Comment 2 • 9 years ago

Another Steps to reproduce:
1. Enable apz
2. Make browser window size to approx. 1000x800px
3. Open https://www.youtube.com/watch?t=10&v=Nnu0A-jIh-U
   --- maybe see the bug

	Alice0775 White Reporter
	Comment 3 • 9 years ago

Reproduced on ubuntu14.04

	Botond Ballo [:botond] Assignee
	Comment 4 • 9 years ago

I can repro this, although for me reloading the page does fix the problem.

	Botond Ballo [:botond] Assignee
	Comment 5 • 9 years ago

I think the problem is stemming from the fact that in HandlePossibleViewportChange(), we zoom by the ratio of two "intrisic scales" [1]. The first is calculated based on the composition bounds, which excludes the scrollbar areas, but the second is calculated based on a saved version of mInnerSize, which does not. The resulting zoom is inaccurate and messes things up.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=256f3225ca4e#376

	Botond Ballo [:botond] Assignee
	Comment 6 • 9 years ago

Clearly the calculation needs to be adjusted so that the intrinsic scales are both calculated the same way. The question is which is the correct way: (mInnerSize / viewport size), or (composition bounds / viewport size)? (With the composition bounds being mInnerSize minus the size of the scrollbars.)

To me, the sensible answer would be the one that keeps the intrinsic scale at 1 when the viewport size tracks the size of the available display area. Currently the thing that viewport size tracks is mInnerSize, but that's  because mInnerSize is what we pass as the input to nsContentUtils::GetViewportInfo() [1], so it just begs the question.

Timothy, should the input to GetViewportInfo() include the exclude non-overlay scrollbar areas?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=256f3225ca4e#283

	Timothy Nikkel (:tnikkel)
	Comment 7 • 9 years ago

(In reply to Botond Ballo [:botond] from comment #6)
> Timothy, should the input to GetViewportInfo() include the exclude
> non-overlay scrollbar areas?
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.
> cpp?rev=256f3225ca4e#283

One thing that we should consider is that the size of the document determines if we get scrollbars or not. Without looking too deep into this it doesn't seem like we would want the return value of GetViewportInfo to depend on whether to document currently has scrollbars or not; we want it to be the same whether the document gets scrollbars or not.

As for your questions about intrinsic scale I'm not really familiar with this quantity, and what its intended use is. Clarifying that would probably help in determining if we want to factor in scrollbars or not for its calculation.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 8 • 9 years ago

I feel like the only scenarios in which the intrinsic scale should matter is on platforms where we support the meta-viewport tag and/or zooming. None of those platforms have non-overlay scrollbars, so maybe we can solve this problem by bailing out of HandlePossibleViewportChange earlier, or otherwise making more things no-ops?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 9 • 9 years ago

(Failing that, I think the intrinsic scale would be better off using the mInnerSize rather than the composition bounds, because as tn said the scrollbars may appear or disappear depending on the length of the content and it probably makes more sense to keep the intrinsic scale constant regardless).

Still, I don't think it makes much sense to consider zooming with non-overlay scrollbars because you could have a non-scrollable document which gets no scrollbars in layout, but then zoom it in the compositor and suddenly need scrollbars to be added. From layout's point of view there should be no scrollbars so we'd have to add them in the compositor, but scrollbars affect the composition bounds in ComputeFrameMetrics, so the composition bounds would be all wrong and so on...

	Botond Ballo [:botond] Assignee
	Comment 10 • 9 years ago

Yeah, based on my discussion with Timothy, it seems that using mInnerSize to calculate the viewport size, and using the ratio of mInnerSize and the viewport size as the intrinsic scale (which should be 1 on platforms without zooming / meta-viewport tags) is correct.

I don't think making things conditional on being on a platform with zooming / meta-viewport tags will make things significantly cleaner - I feel like the correct behaviour for all platforms should fall out from the above change.

	Botond Ballo [:botond] Assignee
	Comment 11 • 9 years ago

/r/9479 - Bug 1167882 - Add utility functions MaxScaleRatio() and MinScaleRatio() to Units.h. r=kats
/r/9481 - Bug 1167882 - Move a couple of static helpers functions higher up in TabChild.cpp. r=kats
/r/9483 - Bug 1167882 - Use the display size (includes scrollbar areas) rather than the root composition bounds (excludes scrollbar areas) to calculate the intrinsic scale. r=kats

Pull down these commits:

hg pull -r 51f87c40889d67a52a4b1635c8e8948c91013637 https://reviewboard-hg.mozilla.org/gecko/

	Botond Ballo [:botond] Assignee
	Comment 12 • 9 years ago

Note that I removed FrameMetrics::CalculateIntrinsicScale() because it didn't make sense there now that one of the inputs is not a field of FrameMetrics. The only call sites were in TabChild, so I just made a static function in TabChild.

	Botond Ballo [:botond] Assignee
	Comment 13 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5135a6bdd07b

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 14 • 9 years ago

Comment on attachment 8611444 [details]
MozReview Request: bz://1167882/botond

https://reviewboard.mozilla.org/r/9477/#review8231

This is much nicer, thanks!

	Botond Ballo [:botond] Assignee
	Comment 15 • 9 years ago

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

Green except for every single Gij test being retriggered twice and eventually failing with "Error: Permission denied to pass object to privileged code", which I assume is unrelated.

	Pulsebot
	Comment 16 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2b770024305
https://hg.mozilla.org/integration/mozilla-inbound/rev/83d5fc938e63
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e86d641a05c

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/d2b770024305
https://hg.mozilla.org/mozilla-central/rev/83d5fc938e63
https://hg.mozilla.org/mozilla-central/rev/2e86d641a05c