[cssom-view] Inconsistencies between partially offscreen scroll-into-view

[bokand]

I've noticed some inconsistencies between engines in how scrollIntoView() and focus() scroll partially offscreen elements.

In https://crbug.com/916631 I removed a magic constant in Blink for elements partially offscreen horizontally - this was inherited from WebKit and meant that an element horizontally onscreen by at least 32 pixels was considered "fully visible" for ScrollIntoView.

However, this led to bugs like https://crbug.com/1036817. I realized that for vertical-only list controls, there isn't a good way to make them clip horizontally (overflow-x: hidden) but navigable vertically; moving focus to a vertically offscreen element should scroll it to view but we shouldn't scroll horizontally.

This appears to have been the intent of the magic constant but I think for element.scrollIntoView makes it hard for authors to reason about hence the above change in Blink (to match Gecko; @emilio added a WPT test for this too).

It seems like it'd make sense to apply this special non-scrolling behavior just for focus()? The focus() spec gives UA some latitude in how the scrollIntoView is done but I wonder if we should try to align behavior between engines? Also, not scrolling technically isn't a valid option for ScrollIntoViewOptions AFAICT.

Interestingly, it appears Firefox does avoid scrolling a partial element onscreen from focus. I'm going to make the same change in Blink but I'm having a hard time exactly determining Gecko's behavior. It looks like there's some kind of threshold too (10px onscreen is scrolled but 50px is not) but testing locally on Linux in FF73 I see focus() scrolls both axes into view whereas (using browserstack) on Windows/Mac it only scrolls the vertical axis. @emilio - can you help me understand what's going on here?

Here's a test page that demonstrates the differences between scrollIntoView and focus with differing amounts of scrollport overlap: https://jsbin.com/sewewam

@smfr for WebKit and whether we can align behavior here.

[emilio]

Interestingly, it appears Firefox does avoid scrolling a partial element onscreen from focus. I'm going to make the same change in Blink but I'm having a hard time exactly determining Gecko's behavior. It looks like there's some kind of threshold too (10px onscreen is scrolled but 50px is not) but testing locally on Linux in FF73 I see focus() scrolls both axes into view whereas (using browserstack) on Windows/Mac it only scrolls the vertical axis. @emilio - can you help me understand what's going on here?

Focus does: https://searchfox.org/mozilla-central/rev/174f1195ec740e8f17223b48018f7e14e6d4e40e/dom/base/nsFocusManager.cpp#2180

scrollIntoView does: https://searchfox.org/mozilla-central/rev/174f1195ec740e8f17223b48018f7e14e6d4e40e/dom/base/Element.cpp#623

So focus() is effectively scrollIntoView passing inline: "nearest", block: "nearest", and with IfNotVisible as the WhenToScroll parameter. That effectively uses the "line size" as threshold to determine whether you should actually scroll:

• https://searchfox.org/mozilla-central/rev/174f1195ec740e8f17223b48018f7e14e6d4e40e/layout/base/PresShell.cpp#3243

Which is determined here:

• https://searchfox.org/mozilla-central/rev/174f1195ec740e8f17223b48018f7e14e6d4e40e/layout/base/PresShell.cpp#3302
• https://searchfox.org/mozilla-central/rev/174f1195ec740e8f17223b48018f7e14e6d4e40e/layout/generic/nsGfxScrollFrame.cpp#4412

Which effectively depends on font-size / line-height / font-metrics etc. I think that's what you're observing, and I don't particularly think that behavior is great.

It seems that flag is only used for focus related things, so focus() and scrolling form controls when auto-filling and focusing them. Blaming that flag it comes from: mozilla/gecko-dev@f40d94a (so pretty old code).

I haven't finished blaming the actual code using the flag and I have to run, but the line-size dependent behavior is already in https://searchfox.org/mozilla-central/rev/7ca5ac579cbcae7983ada5666cd06fe7e845960e/layout/base/nsPresShell.cpp#3995.

I'd be pretty happy of standardizing on something simpler if possible. Hope that helps.