1750959 - Tabbing buttons in an overflowed container doesn't align the button with the bottom of the container.

Status: NEW

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Karl Dubost💡 :karlcow]

Attachment: test case for tabbing through buttons.

(Initially reported by Gooz)

Steps to Reproduce
Tab your way through the buttons until you reach button 3

Expected
Bottom of button 3 aligns with bottom of the container

Actual
Button 3 stays in place and doesn't align with container (while subsequent tabbing aligns the bottom of the buttons with the container)

On both Blink and Webkit, button 3 bottom gets aligned with the bottom of the scrollable context of the container. Note that on Safari you first need to activate navigation by tabulation with System Preferences > Keyboard > Shortcuts > Use keyboard navigation to move focus between controls

Comment 1

[Karl Dubost💡 :karlcow]

Attachment: Results after tabbing to button 3

Safari WebKit top (Release 137 (Safari 15.4, WebKit 17613.1.11.8))
Firefox Gecko middle (98.0a1 (2022-01-18) (64-bit))
Edge Blink bottom (Version 99.0.1139.0)

Comment 2

[Emilio Cobos Álvarez (:emilio)]

This code uses WhenToScroll::IfNotVisible so this seems to be sorta intentional to some extent. We could try to change it to IfNotFullyVisible, or do something more complex, but we should check what other browsers do in similar cases...

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

When I check the history, it reaches bug 178324...
https://bugzilla.mozilla.org/attachment.cgi?id=382498&action=diff

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Finally, I reached bug 372665. Smaug, did you have intention of using "if not visible" except avoiding the crash?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Are you talking about nsHTMLAnchorElement::SetFocus?
My guess is that I changed that to be consistent with nsGenericElement::SetFocus.
Though, the test isn't using any <a> elements.
But obviously that was long ago so I don't remember. Did that patch actually change the behavior here?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to Olli Pettay [:smaug] from comment #5)

Are you talking about nsHTMLAnchorElement::SetFocus?

Oh, I'm sorry, I might have been confused. According to attachment 382498 [details] [diff] [review] (nsFocusManager creation), nsHTMLButtonElement::SetFocus used nsGenericHTMLElement::Focus. And nsGenericHTMLElement::Focus still used NS_PRESSHELL_SCROLL_IF_NOT_VISIBLE. So I need to check the history of nsGenericHTMLElement::Focus more.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

nsHTMLButtonElement::Focus before the focus refactoring:
https://searchfox.org/mozilla-central/rev/6bae8f2ecb18bd3df6ab33f64c44ba47dcfbe191/content/html/content/src/nsHTMLButtonElement.cpp#207

nsGenericHTMLFormElement::DoSetFocus which is called above:
https://searchfox.org/mozilla-central/rev/6bae8f2ecb18bd3df6ab33f64c44ba47dcfbe191/content/html/content/src/nsGenericHTMLElement.cpp#2681

And it uses NS_PRESSHELL_SCROLL_IF_NOT_VISIBLE:
https://searchfox.org/mozilla-central/rev/6bae8f2ecb18bd3df6ab33f64c44ba47dcfbe191/content/html/content/src/nsGenericHTMLElement.cpp#2667

It's introduced by bug 372797's attachment 257663 [details] [diff] [review] (smaug), and the behavior is derived from nsLayoutUtils::ScrollIntoView.

It's introduced by bug 254755:
https://bugzilla.mozilla.org/attachment.cgi?id=205628&action=diff#layout/base/nsLayoutUtils.cpp_sec3

And it's derived from nsFormControlFrame::ScrollIntoView:
https://bugzilla.mozilla.org/attachment.cgi?id=205628&action=diff#layout/forms/nsFormControlFrame.cpp_sec4

oddly, it's introduced by bug 905... There is no details about this change.
https://searchfox.org/mozilla-central/diff/364d22bb8cef7d2aa1b6419118a168dd16fdbd8f/layout/forms/nsFormControlFrame.cpp#367

So Emilio's suggestion would not break the original intention of bug 905.