jwstegemann / fritz2

Easily build reactive web-apps in Kotlin based on flows and coroutines.
https://www.fritz2.dev
MIT License
627 stars 25 forks source link

Popover panel with input button ( and Bootstrap ) #850

Open MrAolen opened 5 months ago

MrAolen commented 5 months ago

Hello,

I have the following issue but I don't know if it's a bug from fritz2 or a bad development.

Description :

I'd like to create a dropdown with a list so that I can select from a list of items. I had the restriction of using Boostrap for the CSS part. But I've run into compatibility problems.

In Boostrap we can use Floating Labels to make our forms a bit more dynamic.

In theory, I wanted to turn my popOverButton into an input text, which would display the drop-down list when clicked. So far so good, but when I dismiss I get a focus reset, which puts the mouse cursor back in the input.

Do you have any idea where this might be coming from?

example

To Reproduce :

I created a repository with some code to have an example.

  1. Run the app ( ./gradlew jsRun )
  2. Open the app
  3. Click on input
  4. Click outside of the input

Desktop :

Lysander commented 5 months ago

I think this is due to the focustrap-Call in the render-Code of PopOver:

    fun render() {
        attr("id", componentId)
        trapFocusWhenever(opened) // this call relies on the default params, see below
    }

Whereas the function definition is as follows:

fun Tag<HTMLElement>.trapFocusWhenever(
    condition: Flow<Boolean>,
    restoreFocus: Boolean = true, // This will put the focus back!
    setInitialFocus: InitialFocus = TryToSet
) {

I would suggest you "clone" the PopOver-component into your project (copy and paste the original code and modify the name -> MyPopOver or alike) and try to change the call with better suited values.

If that works, we could discuss, whether and how we open up the PopOver-API to let the user tune the default behaviour there.

I have the idea, that you want to build some "filtering select-menu" or "ComboBox"? There is an open issue #674 in order to provide this btw. It might be better suited to have some dedicated headless component to adress such components. You are welcome to help us with that :-)

A small extra comment to your code:

        popOver {
            div("form-floating") {
                popOverButton("form-control border border-2", tag = RenderContext::input) {
                    id("searchFruitInput") // this is not so good! 

Pass the id into the main component factory popOver! All the brick-factories below chose their Ids based upon the parents Id. In order to set the correct label.for value, just save the popOverButton into some var to grab it from there or reconstruct it by the main componentId, that is in scope of the main factory - the latter might be more "brittle".

You may have a look at the headless components implementations. Iirc we use the var-pattern there quite a lot for handling relations (for labels and some ARIA-attributes too).

MrAolen commented 4 months ago

Hello,

Thank you for your information and sorry for the late reply.

By following your instructions I was able to correct the focus problem using the following code:

fun RenderContext.popOver(
    restoreFocus: Boolean = true,
    classes: String? = null,
    id: String? = null,
    scope: (ScopeContext.() -> Unit) = {},
    initialize: PopOver<HTMLDivElement>.() -> Unit
): Tag<HTMLDivElement> {
    addComponentStructureInfo("popOver", this@popOver.scope, this)
    return RenderContext::div.invoke(this, classes(classes, PopUpPanel.POPUP_RELATIVE), id, scope) {
        PopOver(this, id).run {
            initialize(this)
            attr("id", componentId)
            trapFocusWhenever(opened, restoreFocus = restoreFocus)
        }
    }
}

I have also started to try to create a specific component for this part. It's starting to work but I'm still waiting to discuss about it with my colleagues.

What's best for the next step : to create a PR and discuss about it or to discuss it beforehand and do an initial review before the PR?

Lysander commented 4 months ago

I have also started to try to create a specific component for this part. It's starting to work but I'm still waiting to discuss about it with my colleagues.

Nice!

What's best for the next step : to create a PR and discuss about it or to discuss it beforehand and do an initial review before the PR?

I would suggest to first think a bit more about the topic. As your use case is some rather complex, dedicated Combobox, I would deny the need to change the PopOver component. I would suggest to drop the PopOver inside your approach and to set up directly upon the PopUpPanel-class, as other Popups like the PopOver-component do. You can bypass such intentional limitations then.

If it is useful to provide such a parameter is imho a separate discussion and would be a separate PR imho. We need defintely to find use cases for PopOver-applications, where this behaviour is needed. Unless we find some reasonable use case, I would prefer to keep the API as simple as possible.

Lysander commented 1 month ago

As PR #856 was solved recently, I would suggest to try this again. The focus-trap was wrongly placed inside the Popover component. So I would assume, that the initial problem is solved by this one.