Popup refactor and extension

[endigo9740]

We've had a number of issues and feature requests reported for the current popup implementation. Likewise we've had a bit of "feature creep" since the inception. Given this I'm going to aim for a refactor of this component utilizing all information we have available to us for this component.

This may include a number of new feature requests:

• Expose Additional Floating-UI Props for popup #1014
• Make it easier to create tooltips (reference #1332)
• Popup hover: target popup disappears when mouseout on parent element #1338
• Popup: Allow passing a reference to an element instead of a string id #1336

I'll also aim to address a number of reported bugs:

• Differentiate popup navigation list focus and selection styles #1345
• Popup does not work after page change when on AppBar #1282
• Popup: Hitting the down arrow after display list of items causes it skip the first item #1337
• Popup is not working fine on live as in dev env. #1379 (potential PR fix)

New issues reported in the thread below:

• Make considerations for popups used within a loop (reference)
• consider a way to support multiple popups on one trigger - ex: tooltip + popup (reference)
• The closeQuery: '' selector should default all child element closing the popup
• Hover popups flicker when using an icon inside a button instead of text (CHRIS: btn-icon works best)
• Document: The target in popup settings must be unique, otherwise this may lead to undesired behavior

If you have new ideas or suggestions related to the popup I'm going to ask that you focus them here for now. If the issues is large enough to require a dedicated ticket I'll split it off as need. But I think we need a "wholistic" approach to these updates to ensure everything continues to work in unison as expected.

---

Not implemented / Not supported

NOTE: I'll move items down here that we don't intend to move forward with or cannot support. I'll response in each ticket with a detailed follow up for this designation

• Add new event type for popup hover-and-click #1327
• Popup state called repeatedly when inside #each block #1323

[endigo9740]

Note: make considerations for popups used in a loop.

Suggested on Discord:
https://discord.com/channels/1003691521280856084/1048010639219642419/threads/1099704955478212628

[endigo9740]

Note: consider a way to support multiple popups on one trigger (ex: tooltip + popup)

Suggested on Discord:
https://discord.com/channels/1003691521280856084/1100073004815351939

[endigo9740]

The closeQuery: '' selector is still not working as intended. This will default to anchor/button, but should be used to clear and provide NO value.

[Mahmoud-zino]

The target in the popup settings must be unique, otherwise when duplicated the first popup defined in the DOM will always be triggered, which could lead to unwanted behavior.

A note in the popup documentation would be great.

[Schroedi]

Skeleton 1.2.5 produces flickering popups when run on my machine but not in a stackblitz playground. Version 1.2.0 does not have this issue.
I tried using the same nodejs version as the stackblitz repl (v16.14.2) but that did not solve the problem.

Attached are videos of the bugged and bug free version.

Here is the test project:
https://stackblitz.com/edit/skeletonlabs-repl-ltvnu4?file=src/routes/+page.svelte

Stackblitz.mp4

Local.mp4

Reporting here as suggested in Discord: https://discord.com/channels/1003691521280856084/1101608283808210944

[Sarenor]

#1415 (comment)
Remember to go with mouseover and mouseleave for hover ;)

[cmjoseph07]

@Schroedi In reference to popup flickering issue:

Seems to only be in issue if you are using an icon or something in the button tag. If you plain text inside of the button tag there is no issue and the hover effect seems to work normally as in 1.2.0.

[endigo9740]

FYI I've updated the original post at the top of this thread. This now denotes all known issues to target and resolve as part of the upcoming update. I think at least couple of these are duplicates or related, but I'll use this list as a reference for test cases. I'll reach out to the user that reported an issue if I need more information.

Note that I do have concerns with replacing the string reference for the target popup with an element reference. This could potentially be a breaking change, which would have to be limited to a major release. But again, I'll do what I can here!

[endigo9740]

Alright folks, I've made a solid dent in the popup rework today. I'm rewriting large portions of the action to lay the groundwork for the upcoming fixes and updates:

• Popup refactor #1421

Preview here:
https://skeleton-docs-git-feat-popup-rework-skeleton-labs.vercel.app/utilities/popups

Per today's update, there are a few notable improvements:

• The action logic is a lot leaner and streamlined, while maintaining the same outwards facing API for params
• Events are limited, but better, including: click | hover | focus-blur | focus-click. I'll document these soon.
• The action now has error handling when it can't find the popup or a required Floating UI module (ex: computePosition)
• More and better examples have been provided on the documentation, with more still to come
• I'm nearly at feature parity with production, and expect to this this sometime tomorrow

Do please note that I have disabled the popups in the App Bar during testing. I will re-enable them soon!

[endigo9740]

I'm happy to report that I've now completed my first pass of the refactor. We should now have feature parity with the production version of the popup, as well as a number of new features and bugfixes.

Preview here:
https://skeleton-docs-git-feat-popup-rework-skeleton-labs.vercel.app/utilities/popups

I'll detail these below in no particular order:

1. We now optionally support additional Floating UI middleware, including: size, autoPlacement, hide, inline. However in testing these appear to alter the default behavior of all popups when enabled, so it's recommend you only enable them as needed and if you know what you're doing. Some refinement may be needed here, but the foundation is now in place.
2. I made considerations for creating a simpler way to create tooltips, however, I think the best option is just to inline in the use:popup params. No changes were implemented on my side for this, users can abstract this on their end if they prefer.
3. The hover event now uses mouseover/mouseleave in favor of mouseenter/mouseout. This works much better in a number of scenarios and seems much less error prone.
4. When focus is on the Trigger element you can now hit Tab/ArrowDown to jump to the popup. This will still auto-focus the first focusable child element within the popup, per ARIA APG guidelines. I've also modified the List element focus style to differentiate this from the active state. This should help prevent users thinking a focused items is actively selected.
5. I also fixed a bug related to Tab/ArrowDown skipping options for Autocomplete options. This now correctly focuses the first item in the list.
6. Trigger elements can now support multiple use:popup actions. For example, this means you can now have both a tooltip and dropdown popups on the same trigger. Previously these would fight one another.
7. I've provided an official recommendation here when generating popups within an #each loop: Popup state called repeatedly when inside #each block #1323 (comment)
8. Fixed a bug with closeQuery: '', this will now prevent child elements from closing the popup. Anchor/button are set by default if no value is provided here.
9. I provided a tip in the hover event type code snippet to prevent child elements, like icons, from closing the hovered popup early. In short, you need to disable pointer events for child elements on your target element (ex: button).
10. Additionally the Action settings now properly update when the popupSetting target changes. This means you can use the same trigger (button) to trigger one or more popups! Just note the current popup will close and the new popup will be reflected the next time you interact with the trigger for a seamless transition.

I've also made great strides to provide more and better documentation top-to-bottom for this feature. There's a lot more examples and the instruction should be much more clear. All requirements are laid out in full. However, I welcome feedback if you find any gaps!

There's a number of items in the original post above that we may not move forward with at this time, either due to technical restrictions or preference. I'll follow up on each of those posts and provide a detailed follow up explaining my thoughts on these.

[endigo9740]

Per the above message, I'm going to ping a number of users here to help review and test the changes to the popups in the pending pull request. This includes everyone that has posted an issue within the threads linked at the top. If you have the ability to pull the PR branch and test this, please do:

@Sarenor @JustBarnt @Mahmoud-zino @Schroedi @cmjoseph07 @DevOfManyThings @saturnonearth @rskvazh @jeromecc @ecejeff @sssuneeth @treo

Again, you can preview the updated documentation here:
https://skeleton-docs-git-feat-popup-rework-skeleton-labs.vercel.app/utilities/popups

View the code changes within the PR here:

• Popup refactor #1421

If anyone is interested in having a version of our package built against this branch to test in your own projects, let me know. I'll be happy to provide that tomorrow!

The goal will be to have this merged into dev branch by EOD Friday so it can be part of next week's release, so please do get your feedback in asap!

[DevOfManyThings]

Following the discussion in #1372 I was looking into the performance impact of event listeners, and was curious if you'd tried using mouseenter instead of mouseover?
Example of number of events fired

(also there's a type in middleware here: https://skeleton-docs-git-feat-popup-rework-skeleton-labs.vercel.app/utilities/popups#middlware)

[Mahmoud-zino]

@endigo9740 you didn't mention using only a unique popup target in the docs. related to #1282

The code handling the target didn't change so maybe you forgot to mention it.

please tell me if I can help, I would be happy to.

[endigo9740]

@DevOfManyThings Number 3 from my list above:

The hover event now uses mouseover/mouseleave in favor of mouseenter/mouseout. This works much better in a number of scenarios and seems much less error prone.

@Mahmoud-zino this is present in the code snippet instruction for the featured demo at the top of the page:

[DevOfManyThings]

@endigo9740 I read that line at least 5 times before commenting and every time read it as now uses mouseover/mouseleave in favor of **mouseover**/mouseout, that makes more sense!