Closed Bug 1601933 Opened 4 years ago Closed 4 years ago

GeckoView::ScrollBy and ScrollTo are broken

Categories

(GeckoView :: General, defect, P1)

Unspecified
All
defect

Tracking

(firefox73 fixed)

RESOLVED FIXED
mozilla73
Tracking Status
firefox73 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(3 files)

When I was auditing our usage of window.inner{Width,Height} for bug 1598487, I noticed we are using the metrics in GeckoViewContentChild.toPixels, apparently it's supposed to be the visual viewport instead of the layout viewport.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7eb6e1d050a208c759710974c7df85f0244a156a

Good catch. Sorry I missed this on my test runs. I didn't adequately exercise the GeckoView tests, clearly. I hope the fix is straightforward.

No problem at all. It's actually hard to notice. The fix is absolutely straightforward, but writing automated tests is hard, the test I added here failed on try intermittently.. :/

The tests I am going to add are really flaky on tries, I figured out the reason. The tests use double-tap-zoom to make visual viewport mismatch with layout viewport. In the failure case, the double-tap-zoom was not processed because the test started running before mZoomConstraints.mAllowDoubleTapZoom is updated in AsyncPanZoomController::UpdateZoomConstraints. I couldn't find good ways to make sure the mAllowDoubleTapZoom is updated in JUnit tests, so I am going to add one sec wait before running the test.

Since bug 1514429 window.inner{Width,Height} don't return the visual viewport
size so once after the content scale changed, i.e. the visual viewport size
doesn't match window inner size, GeckoView::ScrollBy and ScrollTo don't work
as expected. This commit has JUnit tests to generate the situation by using
double-tap-zoom on a small element in the initial visual viewport.

Priority: -- → P1
Pushed by hikezoe.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46e48ee15dd0
Use visual viewport width or height for GeckoView::ScrollBy and ScrollTo. r=botond,rbarker,agi
https://hg.mozilla.org/integration/autoland/rev/4cf75b510765
Use visual viewport size instead of window inner size in PanZoomControllerTest.kt. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/65d449af6342
Rename some ScreenLength APIs to differentiate `visual viewport` from `layout viewport`. r=snorp
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73
Has Regression Range: --- → yes
Keywords: regression
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: