fix: make sure page is not scrolled all way up on load

[FezVrasta]

This should fix the issue reported here shipshapecode/shepherd#744 (comment)

@atomiks can you see any downside of this approach?

[github-actions]

Filename	Size	Change
dist/umd/popper-lite.min.js	2.87 kB	+14 B (0%)
dist/umd/popper.min.js	5.3 kB	+16 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
dist/umd/popper-base.min.js	1.8 kB	0 B

_{compressed-size-action}

[atomiks]

@atomiks can you see any downside of this approach?

All of the failing tests? 🤣

Odd that this has now come up twice in one day 🤔 did some browser behavior change recently? floating-ui/react-popper#336

---

It seems like we'd need to use the strategy option to determine the initial style there, as they did in the react-popper PR, otherwise the calculations are incorrect initially.

[atomiks]

Suggested change

	position: 'fixed',
	position: state.options.strategy,

[chuckcarpenter]

What if you want the absolute strategy, but don't want the scroll to happen? Not sure this would fix our issue.

[atomiks]

Indeed it won't. I think you need to prevent focus before Popper has positioned it (onFirstUpdate and after)

[FezVrasta]

Would visibility: hidden help?

[FezVrasta]

It seems it would help, we'd need to set the popper to visibility: hidden and set it back to visible after we positioned it.

https://codepen.io/FezVrasta/pen/wvaWVyw

[atomiks]

You'll need to unset it on onFirstUpdate as well, where you could do the focusing? 🤷‍♂

https://codepen.io/atomiks/pen/KKpMOyV

visibility: hidden won't scroll to the input. What works best is calling .focus() in onFirstUpdate if you want "autofocus / initialfocus" behavior which will scroll the input if needed, in its correct location.

[FezVrasta]

Test this in the "debug view" (iframes block the autofocus behavior)

https://codepen.io/FezVrasta/pen/wvaWVyw

there's not need for the .focus() call

[chuckcarpenter]

We don't call a focus in our usage, it's just the user is scrolled down the page and then this pushes them to the top. We do a call to scrollIntoView() for the target. Even removing that scrolls the user to the top of the page.

[atomiks]

I'm not convinced we should bother fixing this? Adding the visibility property to applyStyles seems like it may cause problems with user code handling it for opacity animations (e.g. Tippy) (edit: yes just tried this, and it does break them)

The user should always call .focus() after the popper was positioned, never before. They can also set visibility themselves too.

[RobbieTheWagner]

I'm not convinced we should bother fixing this?

@atomiks are you saying the intended popper behavior is to scroll to the top of the page every time before then scrolling down to the popper? That seems less than ideal.

[FezVrasta]

@rwwagner90 I think @atomiks' point is that Popper aims to be a low level positioning library, we tend to defer any non-positioning related problem to the libraries that provide the actual "tooltip features" the mainstream users need.

In this optic, adding a specific fix for this issue, at the cost of limiting some users that need to have more control over the rendering phase of the elements, may be out of scope for the project.

Probably we could leverage on a 3rd party modifier for this kind of use cases though.

[RobbieTheWagner]

@FezVrasta I don't have a ton of context, but I would expect showing a popper always scrolling the page to the top to be a blocker level bug. I think as a user, I would expect it to work and ship with that modifier you are describing out of the box. I think the more in depth tooltip libraries should be able to override that as needed though.

[FezVrasta]

@rwwagner90 I see your point, given the complexity of the library it's always a matter of trade-offs, if we add too much functionality, users will complain about the library weight, if we keep it lean, users will complain about edge-cases not handled out of the box.

As a rule of thumb, anything reported more than a couple of times should be considered to go into the library, for all the other one-off requests, we advise to write ad-hoc modifiers.

To unblock you while we work on a solution (or a "wontfix"), I would suggest to set visibility: hidden; on your popper elements when you create them, and set to visibility: visible (or unset it) inside the onFirstUpdate callback.

---

To @atomiks:

The user should always call .focus() after the popper was positioned, never before. They can also set visibility themselves too.

The problem here is not about .focus() but it's about the autofocus HTML attribute. I think it's pretty important for accessibility reasons to deserve to be included in the core. But I'd like to do that in the least intrusive way.

May you help me understand why this PR is breaking Tippy so that we can think about a solution?

[atomiks]

I don't have a ton of context, but I would expect showing a popper always scrolling the page to the top to be a blocker level bug

This only happens when you focus though... when I search for .focus() I find several results, and none for autofocus.

As I recall, autofocus is not good to use for JS apps and also with Popper. You should only use the focus() method

[chuckcarpenter]

@atomiks you were right and it ended up being an issue with .focus and using onFirstUpdate was the key, with a few other tweaks needed for our library. I appreciate the patience and the codepen examples.

[atomiks]

@FezVrasta I think we can close this