chore: Revert "Revert "fix: scrollIntoView should respect scroll-margin (#8715)"" #9146
Conversation
| let {left: newLeft, top: newTop} = targetElement.getBoundingClientRect(); | ||
| // Account for sub pixel differences from rounding | ||
| if ((Math.abs(originalLeft - newLeft) > 1) || (Math.abs(originalTop - newTop) > 1)) { | ||
| scrollParents = containingElement ? getScrollParents(containingElement, true) : []; | ||
| for (let scrollParent of scrollParents) { | ||
| scrollIntoView(scrollParent as HTMLElement, containingElement as HTMLElement, {block: 'center', inline: 'center'}); | ||
| } | ||
| } |
There was a problem hiding this comment.
Since we added the bypass for chrome, this now also supports alternative alignments, to center the containingElement. This was missing in the previous PR also 👍
| if (!isScrollPrevented) { | ||
| scrollParents.push(root); | ||
| } | ||
| let scrollParents = getScrollParents(targetElement, true); |
There was a problem hiding this comment.
Just noting that this does not behave like the native equivalent, since it skips overflow:hidden scroll parents. This potential issue also existed before this PR, so I don't think it's a blocker.
| if (shouldScrollBlock && block === 'start') { | ||
| y += scrollAreaLeft - scrollPortLeft; | ||
| } else if (shouldScrollBlock && block === 'center') { | ||
| y += (scrollAreaTop + scrollAreaBottom) / 2 - (scrollPortTop + scrollPortBottom) / 2; | ||
| } else if (shouldScrollBlock && block === 'end') { |
There was a problem hiding this comment.
Native scrollIntoView only checks for shouldScroll with the nearest alignment option. Should we also do that?
There was a problem hiding this comment.
I think we can keep this as is, IMO this utility doesn't need to mirror the native behavior and I think we'd prefer more of a scrollIntoViewIfNeeded behavior anyways. Any concerns or thoughts from your end?
There was a problem hiding this comment.
Hm, I'm not sure whether this is a public utility or internal. If it's public I think we should aim for closer parity, but provide an option to opt-in or out of IfNeeded behavior. One other issue where this is also not pairing the native version is that intermediate scrollParents below the scrollView also do not get scrolled. I think it would make sense to push the scrollParents loop into scrollIntoView to also align there.
There was a problem hiding this comment.
honestly I'd classify this more of an internal utility (and thus can be more opinionated) since we never documented it for external usage, but it is a bit of a gray area. Making it a public utility and adding an option for IfNeeded would be nice, but it comes with the extra baggage of committing it to being public API and possibly increasing scope/maintenance depending on how much we'd want it to behave like native (as well as maintain behavior with native if that changes). Happy for the rest of the team to weigh in.
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
Did some preliminary testing and the behavior looks good to me, thanks for the fix! I still need to sit down and go through all the math still haha, but I'll see about getting this into our upcoming test session so we can put it through its paces. Just to share timelines, the team is limited on bandwidth due to some pushes on some end of year docs work so realistically we may not get this in till the new year (which might be when the next release is anyways). Thanks once again for the through fix!
|
@LFDanLu No problem, i figure that implies the same for the pending review of the Carousel's implementation is really quite a lot and I would love to avoid losing most of the detailed implementation insights i currently still got in short term memory 😅 |
|
@nwidynski That's right, the team is currently focused on new docs + getting S2 out of pre-release so we don't have many spare cycles. Thank you for letting me know the Carousel RFC is a higher priority, I'll see if I can get at least a preliminary review by the team squeezed in. |
2 participants
This PR refactors
scrollIntoViewandscrollIntoViewportto supportscrollMargin, while working around the regressions mentioned in #8689 (comment). Additionally, a newinline&blockoption for alignment of a container element has been added.By leveraging bounding rectangles, the internal calculations are now streamlined with
DOMLayoutDelegateand significantly simplified.✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: