Popup Issues

[rChaoz]

Hello! There are a few issues I encountered with the popup utility while I was using Skeleton. I would like to make a PR to address them but it also might break some existing functionality, so I want to know what you think.

First, something simple - this line does not actually remove the listener (same for blur), because a new function is created everytime you write () => close(). I used a variable (const closeFunc = () => close()) to fix this.

Second, middleware are added if they exist in the popup store (example). I don't think this is great, as:
a) it's not documented to work this way and
b) it doesn't allow you to configure them per-popup

I fixed this by using this instead:

// Bundle middleware
const middleware = [
    // https://floating-ui.com/docs/offset
    offset(args.middleware?.offset ?? 8),
    // https://floating-ui.com/docs/shift
    shift(args.middleware?.shift ?? { padding: 8 }),
    // https://floating-ui.com/docs/flip
    flip(args.middleware?.flip),
    // https://floating-ui.com/docs/arrow
    arrow(args.middleware?.arrow ? { element: elemArrow || null, ...args.middleware?.arrow } : { element: elemArrow || null }),
]
const middlewareArgs = args.middleware
if (middlewareArgs) {
    // https://floating-ui.com/docs/size
    if (size && middlewareArgs.size) middleware.push(size(args.middleware?.size))
    // https://floating-ui.com/docs/autoPlacement
    if (autoPlacement && middlewareArgs.autoPlacement) middleware.push(autoPlacement(middlewareArgs.autoPlacement))
    // https://floating-ui.com/docs/hide
    if (hide && middlewareArgs.hide) middleware.push(hide(middlewareArgs.hide))
    // https://floating-ui.com/docs/inline
    if (inline && middlewareArgs.inline) middleware.push(inline(middlewareArgs.inline))
}

This does a few things:

• only add middleware if it's defined in the arguments, instead of adding all of the ones present in the store. Adding all of them is generally not great, especially because not all of them are compatible with each other.
• for the arrow middleware - I think it's best to leave element pre-set (with { element: elemArrow || null, ...middlewareArgs.arrow }), as generally, the user will not be able to pass a reference to the DOM element in the config, I think it's counter-intuitive for the arrow to work correctly and it no longer works when you add some offset to it because the element is no longer set

Finally, the close() function does setTimeout() to disable the popup after the fade-out is finished. This is great, but doesn't account for the fact that it might be opened again in-between the initial close() call and the timeout expiring. You can see this problem in action if you use the "hover" event and you quickly move the mouse away and back to the trigger node - it will disappear even though your mouse is still on it.

I fixed this issue like this:

function close(callback) {
    if (!elemPopup) return
    // Set transition duration
    const cssTransitionDuration = parseFloat(window.getComputedStyle(elemPopup).transitionDuration.replace("s", "")) * 1000
    // Set open state to off
    popupState.open = false
    setTimeout(() => {
        if (popupState.open) return // it was re-opened in the meantime
        // Return the current state
        if (args.state) args.state({ state: false })
        // Update the DOM
        elemPopup.style.opacity = "0"
        // disable popup interactions
        elemPopup.setAttribute("inert", "")
        // Cleanup Floating UI autoUpdate (close only)
        if (popupState.autoUpdateCleanup) popupState.autoUpdateCleanup()
        // Trigger callback
        if (callback) callback()
    }, cssTransitionDuration)
}

This immediately sets the state to 'closed'. Then, the timeout checks if the state is still closed - if not, it does nothing. Also, this allows calling args.state with false directly. I also think here it should be called with true and not popupState.open, but it doesn't really make a difference.

[endigo9740]

Hey @rChaoz, first off thanks for the feedback. I agree with many things raised here. Though I will point out this existing issue:

• Integration: Floating UI React/Svelte (Popups) #1916

And specifically:

While we prep and plan for this major change, we're going to implement a feature freeze on the current popup. This helps prevent redundant effort and avoids pushing out the goalpost even further.

Generally speaking several things are implemented in the way they are because of the manner in which the popup system works. With Floating UI as an optional dependency. So the goal will be to get out of the way of Floating UI and let it do more of the heavy lifting.

This said, I'll likely plan to close your issue soon and group it under the prior issue to avoid duplicates here. No need to echo anything from here over, as this post summarizes things fine. I would also encourage you to subscribe to the linked issue so you can be notified to updates to this.

If you have any questions do feel free to reach out. Thanks!