Closed Bug 1656689 Opened 3 months ago Closed 20 days ago

Keyboard scrolling not working while zoomed in with touchpad

Categories

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

Firefox 81
defect

Tracking

()

VERIFIED FIXED
83 Branch
Tracking Status
firefox83 --- verified

People

(Reporter: hujq, Assigned: tnikkel)

References

Details

Attachments

(2 files, 1 obsolete file)

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.

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

Component: Untriaged → Panning and Zooming
Product: Firefox → Core

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.)

Severity: -- → S3
Priority: -- → P3

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.

Assignee: nobody → tnikkel
Status: UNCONFIRMED → NEW
Ever confirmed: true

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.

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.

Depends on: 1666991

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 on attachment 9177554 [details]
Bug 1656689. Remove PresShell::GetNearestScrollableFrame. r?kats

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

Attachment #9177554 - Attachment is obsolete: true
Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/66c2fb744703
Use GetAvailableScrollingDirectionsForUserInputEvents in nsLayoutUtils::GetNearestScrollableFrameForDirection. r=kats
Status: NEW → RESOLVED
Closed: 29 days ago
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch

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

Flags: needinfo?(tnikkel)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 83 Branch → ---
Regressions: 1667275
Depends on: 1667469
Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8aecf9b4bdc4
Use GetAvailableScrollingDirectionsForUserInputEvents in nsLayoutUtils::GetNearestScrollableFrameForDirection. r=kats
Regressions: 1667636
Status: RESOLVED → REOPENED
Flags: needinfo?(tnikkel)
Resolution: INVALID → ---

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

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.

Depends on: 1668751
See Also: → 1668966

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

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

See Also: → 1668969
Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4cccb84c98e4
Use GetAvailableScrollingDirectionsForUserInputEvents in nsLayoutUtils::GetNearestScrollableFrameForDirection. r=kats
Status: REOPENED → RESOLVED
Closed: 27 days ago20 days ago
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch

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.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.