1818126 - Tab key does not trigger infinite scroll on Youtube video lists

Status: VERIFIED FIXED

(Core :: Layout: Scrolling and Overflow, defect)

Description

[James Teh [:Jamie] (away returning 28 Jan)]

STR:

1. Open this page: https://www.youtube.com/@LGR/videos
2. Take note of the last video link rendered in the DOM. An easy way to get this is to open the web console and paste this command:
   vids = document.querySelectorAll("#video-title-link[href]"); vids[vids.length - 1].textContent
3. Keep pressing tab until the video link from 2) is focused.
4. Press tab twice more to get to the next video link.
   • Expected: The next video link is focused.
   • Actual: Focus hits the browser chrome.
5. Shift+tab twice back to the video link from 2).
6. Press down arrow twice to scroll down a bit.
7. Press tab twice more to get to the next video link.
   • Observe: The next video link is focused.

Impact: This makes it impossible for keyboard and screen reader users to scroll to earlier videos that aren't initially rendered.

This works as expected in Chrome.

I'm not sure whether this belongs here, in DOM: UI event sand focus handling or somewhere else.

Comment 1

[James Teh [:Jamie] (away returning 28 Jan)]

This was reported to me on Mastodon, so this is already impacting users in the wild:
https://hkc.social/@nick/109899409603035123

Comment 2

[James Teh [:Jamie] (away returning 28 Jan)]

Note that as well as keyboard scrolling, this impacts scrolling via accessibility APIs (Accessible::ScrollTo), so screen reader users can't scroll this with browse/review cursors either. However, I suspect fixing the former should fix the latter.

Comment 3

[James Teh [:Jamie] (away returning 28 Jan)]

Curiously, scrolling with JS seems to work:
vids = document.querySelectorAll("#video-title-link[href]"); vids[vids.length - 1].scrollIntoView()
So there must be something different about the scroll events we generate with tab/accessibility that Youtube doesn't like?

Comment 4

[Dennis Schubert [:denschub]]

Ksenia, can you take a look at this?

Comment 5

[Dan Robertson (:dlrobertson)]

(In reply to James Teh [:Jamie] from comment #0)

[...]
Impact: This makes it impossible for keyboard and screen reader users to scroll to earlier videos that aren't initially rendered.

Sorry for the delayed response! Thanks for the detailed STR. It was very easy to reproduce this.

This works as expected in Chrome.

I was also able to see the expected behavior with Chrome, but then I noticed that on tab scroll, when the item is not entirely in view, the scrollIntoView call that chrome must use, must be block: 'center' or something like that because the item ends up around the center of the screen. This is then what causes items afterwards to be rendered in. To verify this I reduced the Chrome window to a much smaller size and Chrome then had the same behavior as us. It was a bit tricky to get the Chrome window to have the right size, but I was eventually able to get it.

I'm not sure whether this belongs here, in DOM: UI event sand focus handling or somewhere else.

I am also unsure. For now lets leave this here, but my best guess is that this could be an issue with scrollIntoView which AFAIK would be Layout: Scrolling and Overflow.

Comment 6

[Botond Ballo [:botond]]

We have a known issue on file for scrollIntoView() with block: center, bug 1826158 -- I wonder if that could be related.

Comment 7

[Dan Robertson (:dlrobertson)]

(In reply to James Teh [:Jamie] from comment #3)

Curiously, scrolling with JS seems to work:
vids = document.querySelectorAll("#video-title-link[href]"); vids[vids.length - 1].scrollIntoView()
So there must be something different about the scroll events we generate with tab/accessibility that Youtube doesn't like?

Per Element.scrollIntoView the default value is {block: 'start'}, which for most screens would cause later videos to be rendered. Note that running the above code block would also cause later elements to be rendered if {block: 'center'} was given. I'm starting to think that the issue is with the scroll events we generate for tab scrolls. I don't know much about how they work, but it seems like we should be generating a scrollIntoView call with something other than {block: 'end'} (which is what I bet we're currently emitting). I think it is important to not that switching to something like using a different value for the block argument would be just a workaround. AFAIK it would still be possible to end up with a tab scroll that should cause more content to be rendered, but does not due to the element size and the current viewport size (similar to what currently happens with chrome in this comment).

Comment 8

[Dan Robertson (:dlrobertson)]

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

We have a known issue on file for scrollIntoView() with block: center, bug 1826158 -- I wonder if that could be related.

Based on what i found in bug 1818126 comment 7, I don't think bug 1826158 is related. I think we probably emit a scrollIntoView() call with {block: 'end'}. If you run the following, more videos should be rendered.

vids = document.querySelectorAll("#video-title-link[href]")
vids[vids.length - 1].scrollIntoView({block: 'center'})

Comment 9

[Dan Robertson (:dlrobertson)]

I think that the scrollIntoView call that could load more content is coming from here. I changed this to WhereToScroll::Center and things seem to look Chrome-like, but the tab scroll continued onto the browser chrome at the same point (regardless of us rendering more content that is tab-scrollable).

Comment 10

[Hiroyuki Ikezoe (:hiro)]

My comment in bug 1535232 comment 0 would be somewhat related. In short not specifying WhereToScroll::Center in nsFocusManager::ScrollIntoView is intentional for "Find in page" function, but for other functionalities I think we have never considered what the preferable behavior is.

Also note that as for Element.focus() the HTML standard spec doesn't define where to scroll, it just mentions

block flow direction position set to an implementation-defined value,

Comment 11

[Dan Robertson (:dlrobertson)]

This bug came up in our APZ planning meeting. Marking this as S2 and we plan to dig into this in our upcoming stabilization sprint.

Comment 12

[Dan Robertson (:dlrobertson)]

After a bit more investigation:

• Chrome definitely centers an element on tab scroll when the element is offscreen
• Element.focus() also centers an element for both Chrome and Safari

So I think we should change WhereToScroll::Nearest to WhereToScroll::Center for the ScrollContentIntoView in nsFocusManager::ScrollIntoView. I did some testing and this gets us to the same behavior as Chrome. That being said, it is possible to cause the unwanted behavior intermittently (same as Chrome).

Comment 13

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1818126: Use WhereToScroll::Center when tab scrolling. r=hiro,botond

Chrome and safari center an element on Element.focus() when the element
needs to be scrolled into view. Update our implementation to do the
same.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09a138b12b3b
Use WhereToScroll::Center when tab scrolling. r=hiro,credential-management-reviewers,sgalich

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/09a138b12b3b

Comment 16

[Rolland Eduard Rus]

Reproduced the issue on Nightly 112.0a1 (2023-02-21) following the STR, on macOS 13, Windows 11 and Ubuntu 22.
Verified it on Nightly 118.0a1 (2023-08-21) Beta 117.0b9 builds on macOS 13, Windows 11 and Ubuntu 22, and can confirm that the issue is fixed.