Framework: Setting component error disables next menu item selection

[OlliTietavainenVaadin]

If a blur listener of a TextField sets the component error of a MenuBar, the next menu selected event doesn't register on the server.

• Occurs on 7.7.15 and 8.6.1, Chrome

Sample code to to reproduce:

public class MyUI extends UI {

    @Override
    protected void init(VaadinRequest vaadinRequest) {
        final VerticalLayout layout = new VerticalLayout();

        MenuBar mb = new MenuBar();
        MenuItem mi = mb.addItem("1. Click this menu item", null, new Command() {
            @Override
            public void menuSelected(MenuItem selectedItem) {
                System.out.println("Menu clicked");
                layout.addComponent(new Label("Menu selected"));
            }
        });

        TextField tf = new TextField("2. Focus this TextField and then click the menu");
        tf.addBlurListener(new BlurListener() {
            @Override
            public void blur(BlurEvent event) {
                mb.setComponentError(null); // needed to reproduce the issue
                System.out.println("Focus lost");
            }

        });
        layout.addComponent(mb);
        layout.addComponent(tf);

        setContent(layout);
    }

    @WebServlet(urlPatterns = "/*", name = "MyUIServlet", asyncSupported = true)
    @VaadinServletConfiguration(ui = MyUI.class, productionMode = false)
    public static class MyUIServlet extends VaadinServlet {
    }
}

Click the menu item to see how it adds a Label to the UI. Then focus on the TextField and click the menu again. Event is not triggered on the server. Click the menu again and the event is received again as normal.

[cdir]

@OlliTietavainenVaadin thank you for creating the issue. https://github.com/vaadin/framework/issues/11308 is obsolete then.

[anasmi]

Hi,

sorry for no news in a long time. Unfortunately, it's not possible to solve the original issue without rewriting the logic of how ErrorMessage is set for the MenuBar.(Not only for it, but also for classes extending AbstractComponent)
The particular problem, is that Error is set within the caption. If there is no caption prior to setting ErrorMessage, then new VCaption is created, which means that hierarchy of the components has changed.
This is problematic due to the fact that BlurEvent is fired before mouse click event is (but after on keyDown event).
This pull request Improve VMenuBar click handling logic, though, has improved the situation by taking the above fact into account. It's possible to delay updateFromUIDL execution by some time, if key is not yet released, which enables setting descriptions and width/height preserving next click action. But since caption is not handled within this method, it doesn’t solve this problem. (The issue with updateFromUIDL method is that we are re-instantiating items there again, changing hierarchy, (https://github.com/vaadin/framework/blob/master/client/src/main/java/com/vaadin/client/ui/menubar/MenuBarConnector.java#L74) so the item for which event was supposed to be fired is gone)
Now (with the fix above in place)it's possible to get the desired behavior by setting caption for the MenuBar. So, if setting caption is an option for you, could you set it?
Otherwise, I have found a CSS-workaround with the setting empty caption. So, if you will add something like this:

MenuBar mb = new MenuBar();
mb.addStyleName("alignSignMenuBar");

and this CSS code to your styles file:

.v-slot-alignSignMenuBar .v-widget{

  display: flex !important;
  flex-direction: row !important;
}
  .v-caption-alignSignMenuBar{
    order:2;
  }

The error indicator will be displayed as there is no caption.
The pull request above was included into both 7.7.16 and 8.7-SNAPSHOT already.
I am sorry that there is no straightforward fix for this issue, but I hope you could take advantage of the workaround proposed above.

[cdir]

Hi @anasmi, for MenuBar our workaround will be to simply not call setComponentError. We do not have any error marker on menu bar but our generic framework does not distinguish between menu bar and other components. So we need to implement this exception in the framework.
But it sounds like the same problem might also occure on other components where a caption is missing? That would be a bigger problem because we often do not use the built-in caption but using a separate label.

[anasmi]

Hi @cdir,
I have been trying to reproduce the issue for other components and managed to do it with, at least, Grid and CheckBoxGroup. So yes, the problem seems to be common. (In this case common since all of those components consists of items)
I've been trying to find the root case and it seems that the problem not even Vaadin related on a general level. If you check this JSFiddle https://jsfiddle.net/c3a4y5dm/ you will notice that blurring out from the first input and clicking the button doesn't fire the change event, but for the second input field it does. And VCaption is inserted using `insertBefore'. (the first case)

On the other hand, implementing ErrorMessages through captions was our decision. I will ask tomorrow for advice on this case from our team if I have overseen something. But if the only one solution still would be rewriting how ErrorMessages are handled, then it's hardly possible to be implemented :/

[cdir]

@anasmi ok, I see. But: in our case we only call setComponentError(null) and component error was null before. Hence there is no need to update or insert anything at all. I think it would be less important when there realy is an error because you can argument that the click might not be intendet if the user had seen the marker before. So: maybe it only needs a little check wether an update is needed or not?

[anasmi]

@cdir could I ask why you are calling setComponentError(null), if the error is already null in you case?

[cdir]

We have a framework (https://github.com/linkki-framework/linkki) for data binding that also handles validation and error handling. For each update, the framework sets the current errors for every field. In most cases, there are no errors, so setComponentError (null) is called. This is necessary if validation errors have previously occurred. At the moment we are not checking if an error has occurred before or not, we just call setComponentError (null). Of course, we can first call getComponentError (), but in other cases (such as updating the value), vaadin also checks the value and only updates the value in the client if it has changed.

[anasmi]

I hope I have a good news, if the case you have specified is that settingComponentError(null) if it was null before.

After the delay in the pull request above, it seems to work now correctly. (At least, using the code snippet above I can't reproduce the problem anymore. My test case is a bit more complicated, as I am setting/removing message opposite to the previous option)
You can check with 8.7-SNAPSHOT version. Is it fixed for you there?

[cdir]

@anasmi thank you, I will check it in vaadin 7.7.16 because our upgrade to vaadin 8 is still ongoing.

[anasmi]

Thanks! Let me know of any result.

Also, just to mention. All the other components will work correctly, if you are calling only setErrorMessage(null) as framework already checks if VCaption is needed or not. (It will not add/remove it if not necessary)
This particular problem was with MenuBar re-creating child components too early.

[anasmi]

@cdir did you have a chance to test it out? Does it work for you after the change?

[anasmi]

Please, if the issue is still reproducible, comment here and we will re-open this ticket. Otherwise, you could create a new one.

This particular issue should be addressed by https://github.com/vaadin/framework/pull/11362 and available in 7.7.16

[cornzy]

@anasmi thank you, I had a lot to do with vaadin 8 migration. For our vaadin 7 customers this will be very helpful. If there is any further problem I will create a new issue.

[anasmi]

No problem! :) Thank you for answering back and definitely create an issue, if the problem is still reproducible.