Doubleclick on buttons may open window twice

[cdir]

In vaadin 8.9.2 I have a button that opens a window as a modal dialog. After the window is opened no interaction with the view behind the window is possible,

However when clicking the button very fast the server is getting two events before the window is opened. This leads to two windows wich may irritate the user or even may cause exceptions in the application.

It could be reproduces with every browser, to reproduce the bug it might be easier to slow down the creation of the window or use debugger. But it also happens in real productive applications.

When opening a modal window I expect that even clicking ultra-fast will always lead to exactly one window.

I think the second event should be blocked when there is already a window opened that is not the parent of the clicked button. That means:

1. if the recursive parent of the click-source (button) is the UI there must be no window
2. if the recursive parent of the click-source is a window, it must be the window in front of all others.

I discussed this solution with @OlliTietavainenVaadin in expert chat as a workaround we might integrate in our code. But it a bit hacky because the contract of UI#getWindows() does not say anything about the order of the windows and I can only guess that the latest one is the window in front. I am also convinced that it is not the expected behavior of framework.

[cdir]

@OlliTietavainenVaadin isn't it expected behavior that no action in the view is performed as long as a window is opened?

[Ansku]

As the label indicates, this is not a bug but a new feature. The Button doesn't know what you might choose to do within a click listener and the Window doesn't know what caused it to be opened, there is no trivial way to detect on the framework level specifically modal Window opening events that originate from a single click listener. Any actions that are performed before the modality curtain is in place can and should go through by default. Opening multiple non-modal Windows with one Button might even be desired behaviour in some applications. However, there is an easy workaround for the problem using setDisableOnClick for the Button and adding to the Window a close listener where you enable the Button again.

Adding a generic debouncing feature could be an option although the actual implementation for such a thing would need to be investigated more closely. It would also have to be something that is enabled specifically, as making such behaviour the default would break backwards compatibility. It might even work better as an add-on rather than a built-in framework feature.

[cdir]

I am building a framework on top of vaadin and hence I do not know more about the action that is desired by the button, that vaadin does. That means there is no way to explicitly disable and enable the button because the click listener do not know anything about the window and vice versa.

However what we all know at a time is that a modal window is opened or not. Of course the fix needs to check whether a window is modal or not.

From the JavaDoc of Window#setModel(boolean)

---

 * Sets window modality. When a modal window is open, components outside
 * that window cannot be accessed.
 * <p>
 * Keyboard navigation is restricted by blocking the tab key at the top and
 * bottom of the window by activating the tab stop function internally.

---

From server side of view the window is opened after the first event. After that event there must be no further event of any compnent outside of the window.

[Ansku]

We cannot prevent people from opening two modal Windows, a completely valid use case would be opening another modal Window from inside the first modal Window without closing the first one. If you know your users don't have such use cases you can implement such limitation in your end, but we can't add that to our framework.

The modality curtain only prevents end users from accessing the components, not server-side modifications to them. The connection between client and server is also not instantaneous, that's why you need to disable the button on the client-side with setDisableOnClick in order to prevent unwanted double-clicks. There isn't enough time to send the first click event to the server and the server response back to the client to block the second click that way. When the second click lands the client-side has no way to know what the first click is going to cause to happen on the server-side, if you don't block the second click with setDisableOnClick the second click can and should be sent to the server-side to be processed, and at that point it's the responsibility of the click listener to decide what should or should not be done.

[cdir]

If you might re-read my issue description, I already pointed out that there might be a modal window within a modal window. But it is not allowed by vaadin contract that any component outside of the modal window can be accessed. But the second click is an access to a component outside of the modal window.

And I already mentioned that I also have the view of a framework, I do not know anything about the action of the button as well. But I know there is a way to prevent the second click by framework when a modal window is opened. I only expect that this behavior is part of vaadin (as the vaadin contract says) and not part of my framework because I think every user want to rely on this contract.

[Ansku]

The contract isn't broken, the second click happens before the modality curtain is in place. This is expected behaviour, not a bug. It causes an unfortunate corner case when the first click is supposed to trigger a modality curtain, but corner cases like this are why setDisableOnClick exists. In my opinion it wouldn't be enough to prevent opening of another modal Window from the same click listener, there could very well be other click listeners or other side effects caused by the initial click listener that also shouldn't happen if you expect that the first click is the only click that ever gets processed.

Off the top of my head I can think of three ways to approach a problem like this:

• To rely that the server-side click listeners keep track of potential modality curtains and self-prevent from triggering any actions when one is in place. That doesn't sound plausible for your use unless you are willing to override all listener-adding functionality to use extended listeners with your own base design instead of direct implementations of the existing interfaces.
• To trigger the modality curtain from the client-side with the first click before server-side is even notified of the click, because it's just not possible to make server-side logic fast enough for that. That would also be a new feature, and better suited to some ModalityCurtainButton add-on than within the framework itself. Things might also get tricky with the Window's own modality curtain but I haven't tried that approach so I'm not sure about all the pitfalls.
• To prevent the click from happening by either immediately disabling the button (which is what setDisableOnClick does), or by preventing the click from getting processed on the client-side by that generic debouncing feature that I mentioned in my initial comment. That feature would only extend the setDisableOnClick functionality in some way, it's still something that would need to be called separately.

Personally I see the third option as the best choice. That means you have at least the following options for your framework:

• No automation, rely on documentation and tutorials to teach people to use the setDisableOnClick and close listener trick. The great thing about this option is that it's relatively easy to notice when you are having these problems and the fix isn't difficult to implement.
• Extend UI to override default addWindow to throw an exception and add a new version that gets both Window and a Button so that you can enforce the setDisableOnClick and the close listener. Breaks existing functionality but is definitely easy to notice. As a significant downside you can't open Windows from anything else than Buttons anymore. alternatively just add the new version without disabling the existing one, in which case you can't guarantee that the feature is noticed but it's easy to take into use even after the fact.
• Implement some new WindowOpeningButton that gets a Window as a parameter (so that it supports also classes that extend Window), or maybe even alternatively some extended Window implementation (that can be further extended) that provides the opening Button by default. Then you can have a setDisableOnClick and adding and removing of the close listener that re-enables the Button built-in and you don't even need the debouncing feature. Could get a bit cludge-y and both options again need your users to notice that the new features are there, but new components are somewhat more visible than new methods and this is again a relatively easy change to make once they do notice.
• The same as previous options but using the potential debouncing feature if it ever happens.

It's probably not a good idea to extend the default Button so that you can configure either setDisableOnClick or the potential debouncing feature to be on by default, because that would break many other use cases from your framework in turn (anything that relies on double-clicks to start with, as well as anything that people might want to click fast multiple times).

I understand that none of these things might be quite ideal from your point of view, but we cannot get rid of the inherent delay within a server roundtrip and simply preventing the second window from appearing would only fix the most visible symptoms, not the base issue that the second click still happens and gets processed on both client- and server-side.

The debouncing feature is an interesting concept but it would still need quite a bit of design before I can even be sure how plausible it would be to implement. I suppose one basic idea could be that the first click disables the button on the client-side, and server-side automatically enables it again after all click listeners have been processed, but even that would not guarantee that the triggered actions are actually fully processed by the time the Button is enabled again. On the other hand, you might not even want them to be. And you could still use the default setDisableOnClick trick on the side and trigger the re-enabling yourself whenever it suits you best.

[cdir]

I do not agree but I played around with setDisableOnClick. Seems like I can implement such debouncing mechanism by setting setDisableOnClick and make sure that the button gets it's previous enabled state in a generic click listener. It is not perfect becuase the UI flickers a bit when switching to disabled and enabled back again.

[Enerccio]

This is NOT expected OR desired behavior at all! I am not sure why are you trying to excuse it as a feature. You should, if modal window is opened, disable ALL further events that are coming from components not in that modal window and are from client (not server side events). How would that be hard to implement?

I honestly don't even know how to "fix" it in application code because due to good abstraction buttons are nowhere NEAR the place that opens a window. Or executes any click listener.

[Ansku]

All user interactions are indeed disabled for components beneath modality curtain once the modality curtain is in place, the problem here is that it's possible to click for a second time during the brief delay before the modality curtain is actually added, and I explained above why the framework cannot block that for you without the disableOnClick feature. However, you can get the button reference from the ClickEvent, so that gives your Window an easy access to know which button it needs to re-enable from the close listener -- assuming that your click listener knows that it's about to open a modal Window. If it doesn't, I'm afraid the next best option is to carry the Button reference to whatever location does decide whether the Window should be opened or not. Of course you still need to know which button to apply the disableOnClick for before the first click happens, and that one might likewise require modifying your abstraction level to carry that variable along one way or another.

[Enerccio]

and I explained above why the framework cannot block that for you without the disableOnClick feature

You really didn't because framework could easily do that. In fact, I "solved" it by doing this:

public class FixedNativeButton extends NativeButton {

    public FixedNativeButton() {
    }

    public FixedNativeButton(String caption) {
        super(caption);
    }

    public FixedNativeButton(String caption, ClickListener listener) {
        super(caption, listener);
    }

    @Override
    public void addClickListener(ClickListener listener) {
        super.addClickListener(new ModalWindowCheckingListener(listener, UI.getCurrent()));
    }

    private class ModalWindowCheckingListener implements ClickListener {

        private final UI ui;
        private final ClickListener listener;

        public ModalWindowCheckingListener(ClickListener listener, UI ui) {
            this.ui = ui;
            this.listener = listener;
        }

        @Override
        public void buttonClick(ClickEvent event) {
            if (isModalWindowOpened() && !buttonBelongsToLastModal()) {
                return;
            }

            listener.buttonClick(event);
        }

        private boolean buttonBelongsToLastModal() {
            Window last = null;
            for (Window window : ui.getWindows()) {
                last = window;
            }
            if (last != null) {
                if (FixedNativeButton.this.isOrHasAncestor(last)) {
                    return true;
                }
            }
            return false;
        }

        private boolean isModalWindowOpened() {
            return ui.getWindows().stream().anyMatch(Window::isModal);
        }
    }

}

which honestly is what framework should do. The edge case where there are multiple click listeners on a button (who does that, really) could be handled by special flag for those rare events.

Honestly, framework should do this filtering on the lowest level, ie when it is parsing events from client. Simply check if there is modal window open and filter all events that are not from component from that modal window. It would be simple enough to do and would be correct.

If it doesn't, I'm afraid the next best option is to carry the Button reference to whatever location does decide whether the Window should be opened or not. Of course you still need to know which button to apply the disableOnClick for before the first click happens, and that one might likewise require modifying your abstraction level to carry that variable along one way or another.

This is a ridiculous "solution" since that means every click interaction would propagate button everywhere because you simply do not know, at click time, if it will open window. So you have to propagate button everywhere for some reason. Instead of fixing it on framework level so it actually works like advertised.

[Ansku]

Yes, extending the Button is a potential workaround, but as I mentioned we can't do it like that on framework level. Even if we ignore the potential performance hit and the problem with multiple listeners (I would personally deem that use case more common than this problem situation here), a solution like that would also break the use case where something completely unrelated to the button triggers a modal window on the server side just when you are clicking on the client side.

If I click a button that says 'Save changes' and a heartbeat later a modal window appears to inform me about something completely unrelated (say, a warning that my session is about to expire), I would definitely assume that my changes did in fact get saved regardless and be very upset if they didn't. In some applications the freshly added modal window might even get automatically closed before the modality curtain is ever applied to the client-side (e.g. because the recent button click makes the session expiration warning unnecessary), at which point the server-side block would just silently eat your click event with no hint given to you on what's going on. Both of these problem situations would be far worse than this current problem, and much more difficult to work around.