1719330 - Using arrow keys in scrollable container scrolls by more than 1 element at a time

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[ldebeasi]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.114 Safari/537.36

Steps to reproduce:

1. Open the following CodePen in Firefox: https://codepen.io/liamdebeasi/pen/OJmNJyL
2. Click into the scrollable container.
3. Press the "Down" arrow key. Observe that the scrollable container scrolls from item 0 to item 2.

Additional Information:

• The height of the scrollable container is set to 28px. If I set it to 34px the issue does not happen, though I am not quite sure why that is.
• I can reproduce this in Firefox Nightly v91 as well.
• This issue does not reproduce in Chrome.
• This issue does reproduce in Safari 14.1, but does not reproduce in Safari Technology Preview so I think the issue was fixed there.

Actual results:

Pressing the arrow key scrolled the container by more than 1 element.

Expected results:

I would expect that the scrollable container scrolls by only 1 element at a time.

Comment 1

[Alice0775 White]

I can reproduce the issue on Windows10.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

So this is basically we use 3 lines for line scrolling. Should we restrict the scroll amount if it's larger than the scroll port size? It looks like both Chrome and Safari do the restriction.

Maybe we could just fallback to eScrollPage case here in AsyncPanZoomController::GetKeyboardDestination?

Comment 3

[Botond Ballo [:botond]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)

Should we restrict the scroll amount if it's larger than the scroll port size?

That sounds fine in principle. As with other keyboard scrolling bugs, we should check the behaviour with apz.keyboard.enabled=false.

Comment 4

[b.m199802]

Hello ! I'm facing this issue on my app. Can you say me if this bug has been resolved, or if it is gonna be ?
Thanks :)

Comment 5

[Botond Ballo [:botond]]

This bug is not on our short-term roadmap, but it sounds like the fix described in comment 2 should be fairly straightforward. If you'd like to give it a try, I'm happy to provide guidance.

Comment 6

[Razvan Cojocaru]

I've tried to debug this a bit, and in spite of comment 2, I can't get AsyncPanZoomController::GetKeyboardDestination() to be called at all (I've tried apz.keyboard.enabled set to both true and false). Is this a driver thing again? What am I missing?

Comment 7

[Botond Ballo [:botond]]

Keyboard events are a bit different from other input events in that we can only handle them in APZ in some cases.

So we have two codepaths for handling keyboard events, a main-thread codepath and an APZ codepath, and we try to use the APZ codepath on a best-effort basis, but often fall back to the main-thread codepath.

So, if you're not seeing the APZ codepath get hit in this case, we must be falling back to the main-thread codepath for some reason. I believe the main-thread codepath for up/down arrow keyts is PresShell::ScrollLine (which is hooked up to keyboard commands here). (If you're curious why we don't hit the APZ codepath in this case, you can trace through the code here and see which early-exit is taken.)

The fix will need to be made to both codepaths (assuming both are affected).

Comment 8

[Razvan Cojocaru]

(In reply to Botond Ballo [:botond] from comment #7)

Keyboard events are a bit different from other input events in that we can only handle them in APZ in some cases.

So we have two codepaths for handling keyboard events, a main-thread codepath and an APZ codepath, and we try to use the APZ codepath on a best-effort basis, but often fall back to the main-thread codepath.

So, if you're not seeing the APZ codepath get hit in this case, we must be falling back to the main-thread codepath for some reason. I believe the main-thread codepath for up/down arrow keyts is PresShell::ScrollLine (which is hooked up to keyboard commands here). (If you're curious why we don't hit the APZ codepath in this case, you can trace through the code here and see which early-exit is taken.)

The fix will need to be made to both codepaths (assuming both are affected).

Thanks! I can confirm that that's where I land in the code, and also that changing lineCount to 1 (instead of the default value, 3) appears to fix the issue. Now I need to figure out what the code equivalent of "scroll amount larger than the scroll port size" is. In AsyncPanZoomController.cpp I've compared Metrics().GetCompositionBounds().Height() to scrollDistance * lineScrollSize.height, but I didn't get to test it since that code path wasn't hit. I'm unsure of what the correct condition is in PresShell.cpp.

Comment 9

[Botond Ballo [:botond]]

In the main-thread codepath, the scroll port can be obtained using nsIScrollableFrame::GetScrollPortRect(). (The "composition bounds" in APZ is the scroll port rect, just in a slightly different coordinate system.) The height of a line can be obtained using nsIScrollableFrame::GetLineScrollAmount().

Comment 10

[Razvan Cojocaru]

(In reply to Botond Ballo [:botond] from comment #7)

So, if you're not seeing the APZ codepath get hit in this case, we must be falling back to the main-thread codepath for some reason. I believe the main-thread codepath for up/down arrow keyts is PresShell::ScrollLine (which is hooked up to keyboard commands here). (If you're curious why we don't hit the APZ codepath in this case, you can trace through the code here and see which early-exit is taken.)

It looks like this is the early exit, leading to APZInputBridge::ReceiveInputEvent (with input.mHandledByAPZ always false). What should I do to make sure I end up in AsyncPanZoomController::GetKeyboardDestination()?

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Maybe this comment help?

Comment 12

[Botond Ballo [:botond]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)

Maybe this comment help?

Yes, this is very helpful -- thanks!

(In reply to Razvan Cojocaru from comment #10)

It looks like this is the early exit

Based on the comment Hiro linked, I would guess that the key event which takes that early exit has keyInput.mType == KEY_DOWN, and the key press also results in a second event with keyInput.mType == KEY_PRESS, which takes a different early-exit?

What should I do to make sure I end up in AsyncPanZoomController::GetKeyboardDestination()?

Do subsequent key presses (i.e. not the first one, but starting from the second) get to AsyncPanZoomController::GetKeyboardDestination()? That would be consistent with the findings in bug 1774519.

Also, are you testing on codepen.io? Are the results any different if you instead copy the HTML/JS/CSS snippets into a standalone HTML file, and test that? Whether we hit the APZ codepath for keyboard scrolling is sensitive to things like the presence of key event listeners, so maybe codepen.io has some key event listeners which interfere.

Comment 13

[Razvan Cojocaru]

(In reply to Botond Ballo [:botond] from comment #12)

It looks like this is the early exit

Based on the comment Hiro linked, I would guess that the key event which takes that early exit has keyInput.mType == KEY_DOWN, and the key press also results in a second event with keyInput.mType == KEY_PRESS, which takes a different early-exit?

What should I do to make sure I end up in AsyncPanZoomController::GetKeyboardDestination()?

Do subsequent key presses (i.e. not the first one, but starting from the second) get to AsyncPanZoomController::GetKeyboardDestination()? That would be consistent with the findings in bug 1774519.

Also, are you testing on codepen.io? Are the results any different if you instead copy the HTML/JS/CSS snippets into a standalone HTML file, and test that? Whether we hit the APZ codepath for keyboard scrolling is sensitive to things like the presence of key event listeners, so maybe codepen.io has some key event listeners which interfere.

I was indeed testing on codepen.io, and it turns out that that was really the problem. I can get to the APZ code from files-on-disk.
I guess the good part of this is that I was able to modify, and test the fix for, the PresShell code.

Comment 14

[Razvan Cojocaru]

Attachment: Bug 1719330 - Limit line-scrolling to one page in scrollable elements with a short viewport. r?botond,hiro

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48dcd2581007
Limit line-scrolling to one page in scrollable elements with a short viewport. r=botond

Comment 16

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/48dcd2581007