Fix Manipulator.offset()

[alpadev]

fixes #32496, closes #32341

Background

Manipulator.offset() is using getBoundingClientRect() that returns top/left values relative to the viewport. The current implementation uses document.body.scrollTop and document.body.scrollLeft to get the scroll position of the viewport. (Both returning 0 in current Chrome and FF)

This used to work with older browsers but since then got changed to document.documentElement.scrollTop and document.documentElement.scrollLeft. The currently proposed way to get those values (according to MDN) is to use window.pageYOffset and window.pageXOffset.

TBD

• Should we also add fallbacks for window.pageYOffset and window.pageXOffset mentioned on the MDN page?

• Not sure about the test cases. Maybe someone got a better idea how to implement them.

Preview: https://deploy-preview-33603--twbs-bootstrap.netlify.app/docs/5.0/components/scrollspy/

Example of #32496 using the fix: https://codepen.io/-alpa-/pen/MWpVooG

[GeoSot]

Alpa, sure is better than before. However check the preview. It fails to initialize in proper element after refresh
EDIT: It starts in the right position. It fails on next scroll, making me suspect the internals of scrollspy

[PetersOtto]

Is that issue really fix? I have the same behavior than before. #32496
Perhaps I do something wrong but I think the issue is not gone.

[harnishdesign]

Issue is still in bootstrap v5.0.1. #32496 have available finally any fixed?

[arcanisgk]

excuse my ignorance; but if a single DOMContentLoaded is placed; What will happen to the content added with bootstrap ajax, what is expected to have any interaction or effect, will it still work?

[alpadev]

@arcanisgk how is that related?

[GeoSot]

@alpadev Correct me if I am wrong, but Manipulator methods offset & position are used only on scrollSpy, So they maybe can be moved to it

[XhmikosR]

@alpadev regarding your OP TODO

1. I don't think our supported browsers don't support scrollY so we should be safe
2. Not sure about the tests, I trust you, @rohit2sharma95 and @GeoSot :)

If you don't have further changes, we could include this in v5.0.2

[XhmikosR]

Forgot to mention that if we already use pageYOffset/pageYOffsetY, we could probably use that here too and switch to the other in a separate PR.

[XhmikosR]

@alpadev it seems we are getting random BrowserStack failures:

Mobile Safari 12.1.1 (iOS 12.3.1) Manipulator offset should not change offset when viewport is scrolled FAILED
	Error: Timeout - Async function did not complete within 5000ms (set by jasmine.DEFAULT_TIMEOUT_INTERVAL) in node_modules/jasmine-core/lib/jasmine-core/jasmine.js (line 7741)
	<Jasmine>

Maybe we need some timeout?

[arcanisgk]

It seems to me not to be completely random, it simply tries to execute at the same time as the resizing/changing the orientation of Mobile, it is not that it needs a time out I think it needs to know the status of the viewport.

[XhmikosR]

By random I mean it doesn't always happen as you can see by the current test status :)

Anyway, if anyone has any patches, please make a new PR.

[arcanisgk]

I am almost certain that the event is intercepted in the rotation of a Mobile device:

var supportsOrientationChange = "onorientationchange" in window,
    orientationEvent = supportsOrientationChange ? "orientationchange" : "resize";

window.addEventListener(orientationEvent, function() {
    alert('HOLY ROTATING SCREENS BATMAN:' + window.orientation + " " + screen.width);
}, false);

[XhmikosR]

@arcanisgk could you make a PR please? We keep hitting the CI failure but it's not 100% reproducible.

[arcanisgk]

@XhmikosR Hello, I'm sorry to tell you that I don't have the slightest idea what "PR" is; The most I have achieved in github is to create a personal repository upload a library and publish it to be distributed in composer, I would like to help more but I do not have enough knowledge to support them.

[XhmikosR]

No worries. We need someone to have a look at this issue at some point. I might create a new issue so that this gains more eyes.