1656689 - Keyboard scrolling not working while zoomed in with touchpad

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[hujq]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:81.0) Gecko/20100101 Firefox/81.0

Steps to reproduce:

Set apz.allow_zooming to true.
Navigate to any web page, e.g. https://en.wikipedia.org/wiki/Main_Page .
Pinch to zoom-in with touchpad.
Try to scroll on the page with arrow keys on the keyboard.

Actual results:

For horizontal (left and right arrow key), the page is not moving at all.
For vertical (up and down arrow key), it will start scrolling, but at the same time jump to the leftmost side of the page. It also won't scroll past a certain point towards the bottom of the page.

Expected results:

It should be possible to use arrow keys to scroll over the whole page, without jumping or cutoff.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Botond Ballo [:botond]]

I think this boils down do "main-thread keyboard scrolling code needs to scroll the visual viewport".

(Marking as blocking desktop-zoom-nightly for triage purposes only, I'm not necessarily arguing that it needs to block nightly.)

Comment 3

[Timothy Nikkel (:tnikkel)]

I expect that the code added in bug 1651332 fixed this, but that code is currently disabled because the pref apz.force_disable_desktop_zooming_scrollbars is set to true by default.

Comment 4

[Timothy Nikkel (:tnikkel)]

This is only partially fixed by the flipping apz.force_disable_desktop_zooming_scrollbars to false. There is another issue, patch coming up for it.

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656689. Use GetAvailableScrollingDirectionsForUserInputEvents in nsLayoutUtils::GetNearestScrollableFrameForDirection. r?kats,botond

Keyboard arrow key scrolls go through PresShell::ScrollLine/ScrollCharacter. When these look for a scroll frame they eventually end up at nsLayoutUtils::GetNearestScrollableFrameForDirection.

nsLayoutUtils::GetNearestScrollableFrameForDirection does two things wrong when determining if the scroll frame is scrollable by apz: it disallows overflow hidden scrolling, and it uses GetAvailableScrollingDirections which is based on the layout scroll range.

Using GetAvailableScrollingDirectionsForUserInputEvents takes into account both of these factors.

GetNearestScrollableFrameForDirection is used by:
-gfx/layers/apz/src/FocusTarget.cpp
-EventStateManager::DoContentCommandScrollEvent
-PageMove, ScrollPage, ScrollLine, ScrollCharacter, CompleteScroll of PresShell
All of these should want the more apz aware function.

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656689. Add test. r?kats

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656689. Remove PresShell::GetNearestScrollableFrame. r?kats

The current formulation is inconsistent. If you call it with ScrollableDirection::Either it goes into nsLayoutUtils::GetNearestScrollableFrame which will return the first scroll frame with a non-hidden overflow. If you call it with any other ScrollableDirection it calls nsLayoutUtils::GetNearestScrollableFrameForDirection which returns the first scrollframe it finds that has non-hidden overflow AND has at least one dev pixel of scroll range.

So remove that function and call nsLayoutUtils::GetNearestScrollableFrameForDirection directly. This is a slight change of behaviour but it seems desirable for all callers.

Comment 8

[Phabricator Automation]

Comment on attachment 9177554 [details]
Bug 1656689. Remove PresShell::GetNearestScrollableFrame. r?kats

Revision D91236 was moved to bug 1666991. Setting attachment 9177554 [details] to obsolete.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66c2fb744703
Use GetAvailableScrollingDirectionsForUserInputEvents in nsLayoutUtils::GetNearestScrollableFrameForDirection. r=kats

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d036cd1b2a14
Add test. r=kats

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/66c2fb744703

Comment 12

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on test_group_zoom-2.html

backout: https://hg.mozilla.org/integration/autoland/rev/90e7c137c0edc740f29424745a1252b874b0d4f0

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=dbtVajvcQ4-G2htWRvYmvA.0&revision=d036cd1b2a140d1e4c474fbf6197181ea0c34448

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=316689719&repo=autoland&lineNumber=12048

[task 2020-09-25T07:02:01.940Z] 07:02:01 INFO - TEST-PASS | gfx/layers/apz/test/mochitest/test_group_zoom-2.html | helper_zoom_keyboardscroll.html | shouldn't have scrolled
[task 2020-09-25T07:02:01.942Z] 07:02:01 INFO - Buffered messages finished
[task 2020-09-25T07:02:01.943Z] 07:02:01 INFO - TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_group_zoom-2.html | helper_zoom_keyboardscroll.html | shouldn't have scrolled - got 8, expected +0
[task 2020-09-25T07:02:01.943Z] 07:02:01 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:500:14
[task 2020-09-25T07:02:01.944Z] 07:02:01 INFO - spawnTest/w.is@gfx/layers/apz/test/mochitest/apz_test_utils.js:420:18
[task 2020-09-25T07:02:01.944Z] 07:02:01 INFO - test@gfx/layers/apz/test/mochitest/helper_zoom_keyboardscroll.html:41:9
[task 2020-09-25T07:02:01.947Z] 07:02:01 INFO - driveTest@gfx/layers/apz/test/mochitest/apz_test_utils.js:644:36
[task 2020-09-25T07:02:01.947Z] 07:02:01 INFO - TEST-PASS | gfx/layers/apz/test/mochitest/test_group_zoom-2.html | helper_zoom_keyboardscroll.html | shouldn't have scrolled

Comment 13

[Natalia Csoregi [:nataliaCs]]

Backed out 66c2fb744703 for causing Bug 1667275

backout: https://hg.mozilla.org/integration/autoland/rev/11d61fa7276a0b9b0fda8e346d5cbc97f061b097

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8aecf9b4bdc4
Use GetAvailableScrollingDirectionsForUserInputEvents in nsLayoutUtils::GetNearestScrollableFrameForDirection. r=kats

Comment 15

[Atila Butkovits]

Backed out for causing high failure rate on test_scroll_space_no_range_overflow_scroll.html.

Backout link: https://hg.mozilla.org/integration/autoland/rev/4668c7b5e842a7afef9e4c183c8a85af950eb1ad

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8aecf9b4bdc4e737d5de27e7a264ec3a7930a3f0&selectedTaskRun=Dn5JKY6rSgq3qIKvqbfNiA.0&searchStr=Android%2C7.0%2Cx86-64%2Cdebug%2CMochitests%2Ctest-android-em-7.0-x86_64%2Fdebug-geckoview-mochitest-plain-e10s%2C2

Comment 16

[Timothy Nikkel (:tnikkel)]

This new failure in test_scroll_space_no_range_overflow_scroll.html is fun. The test calls pageMove. PresShell::PageMove calls GetNearestScrollableFrameForDirection. The root scroll frame of the test is scrollable (there is a 200 vh spacer in it), however when run within the mochitest testing harness iframe it is not scrollable (ie GetScrollStyles returns overflow hidden) because we set scrolling=no on it here

https://searchfox.org/mozilla-central/rev/0eebca3c33b7999bfd090672e6c6dde96ae89616/testing/mochitest/server.js#824

And the blame for that line appears to go back to 2007 when we first introducted mochitest. Long time viewers may remember bug 1199023, where I discovered that mochitest-chrome was using scrolling=no and removed it. I guess I didn't find this use of scrolling=no because it is applying the scrolling=no in a different way that a search wouldn't be able to easily find. If I remove that we fail maybe around 10ish tests:

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

Now, back to the problem at hand. So the root scroll frame of the test is determined to not be scrollable and so we traverse to the parent document, the one housing the mochitest harness. Before the patch of this bug we determine that there is no layout scrolling that can be done on the root scroll frame of the mochitest harness document and return null. After this patch we determine that there is apz scrolling that can be done and so return the root scroll frame of the mochitest harness document.

Then, without this patch, since we got a null frame we call nsFrameSelection::GetFrameToPageSelect to get the frame to use. With this patch we have a frame. Now we call nsFrameSelection::PageMove with the frame that we have. nsFrameSelection::PageMove calls GetOffsetTo between the passed in frame and the frame that the caret is in (the focused frame). Before this patch this is the root frame of the test document and a div in the test document. After this patch this is the root frame of the mochitest testing harness parent document, and a div in the test document. GetOffsetTo is not to be used on frames in different documents so this asserts, and then I think we try to scroll the test harness document, so the test doesn't see the scroll it is expecting.

So we need to remove scrolling=no from the mochitest iframe, but that looks to be a bit time consuming dealing with multiple tests and potential fallout. And we need to fix this code, as it is broken even with that problem. That seems a little more tractable in the short term to get this bug fixed. Initial question in my mind for that task is: if we don't find a scrollable scroll frame in the document of PresShell, should we ignore the PageMove call or forward the PageMove to a parent PresShell? Reading the description of the function in the idl it sounds like we should ignore it

https://searchfox.org/mozilla-central/rev/670e13b51d272125c76a1bf84b9f3707583cde12/dom/base/nsISelectionController.idl#239

because it is specifically dealing with the selection in one specific document. But it looks like there are two things going on: expanding the selection and scrolling. And while the selection may be limited within some subtree, the scrolling looks like it can maybe go above that:

https://searchfox.org/mozilla-central/rev/670e13b51d272125c76a1bf84b9f3707583cde12/layout/generic/nsFrameSelection.cpp#1857

Comment 17

[Timothy Nikkel (:tnikkel)]

The resulting ScrollFrameRectIntoView call looks like it will scroll all ancestors no matter what, so I think we just need to make sure the frames we pass to nsFrameSelection::PageMove are in the same document.

Comment 18

[Timothy Nikkel (:tnikkel)]

Bug 1668966 fixes an issue that comes up with the test here.

Comment 19

[Timothy Nikkel (:tnikkel)]

Bug 1668969 is filed the the scrolling=no on mochitest harness iframe issue.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4cccb84c98e4
Use GetAvailableScrollingDirectionsForUserInputEvents in nsLayoutUtils::GetNearestScrollableFrameForDirection. r=kats

Comment 21

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4cccb84c98e4

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05b6055fd7f3
Add test. r=kats

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/05b6055fd7f3

Comment 24

[Hani Yacoub, Desktop QA]

Verified as fixed on Windows 10 ARM, Windows 10 Surface Pro Touchscreen, Dell laptop with Windows 10, MacOS 10.15 on Firefox Nightly 83.0a1.