emilio
commented
4 years ago 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?
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:
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: https://github.com/mozilla/gecko-dev/commit/f40d94ae94b1d7a1c69c43f1406c52f8934059fb (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.
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.