bug #22 TimePicker in Grid causes TypeError

[WoozyG]

the problem is the FIXME lines in timepickerConnector.js

• they access the shadow DOM, but it hasn't been inititalized when setting the locale upon initial attachment.

This solution just makes a dumy JS void(0) call, and upon success, asynchronously calls setLocale(). This allows the client to fully initialize the element and nodes before attempting to directly acccess them in an unsafe manner.

I'm sure there is a better way, thus the "FIXME" comment in there, but as this is my first foray into Vaadin 10+, I'll stick to the server side.

This can be verified as correct by setting the locale before the sample code in the issue:

UI.getCurrent().setLocale(Locale.CHINA);

and observing that the time is displayed with the proper format upon first render in the UI.

---

This change is 

[CLAassistant]

All committers have signed the CLA.

[tomivirkki]

HI @WoozyG, thanks for the contribution. Can you also add a test case to validate the bug and the fix for it?

[WoozyG]

HI @WoozyG, thanks for the contribution. Can you also add a test case to validate the bug and the fix for it?

I'm not familiar enough with TestBench to know if it is even possible to trap the original browser-side errors and report them or not. Current tests won't show anything, as they are only on the Java side. The Java side will report the proper locale, even if the client side fails to update the locale in the browser component.

I can't think of a way to test the original error unless there is a TestBench method I'm not aware of that can capture uncaught browser exceptions during component initialization, or for that matter test that the client side component locale matches the server side component locale.

The reason the current tests pass is that the TimePicker component is created and attached in page initialization, and then all the tests set the locale in subsequent calls. The error case is setting a non-default locale in the Page or Session before attaching the TimePicker component. In that case, which includes cases where a user's browser settings indicate a locale other than "en-US" the initial display of the component will be incorrect.

[WoozyG]

The new test passes, but I'm unsure if it would fail or not on the prior code - I think it would fail, as the label and the component value would display differently, but I'm not confident in my reading of the order of operations and request/response flow in the integration tests. It definitely checks the new code here.

[alvarezguille]

Hi @WoozyG,

I can't think of a way to test the original error unless there is a TestBench method I'm not aware of that can capture uncaught browser exceptions during component initialization, or for that matter test that the client side component locale matches the server side component locale.

You can add checkLogsForErrors() to the AbstractComponentIT to check for error logs, it's not part of TestBench yet, but it's part of a helper we use in components.

Have you been able to reproduce the issue just with TimePicker? I wasn't able to reproduce it, with the test view you provided when undoing the logic changes :(. This makes the regression test not really good.

[WoozyG]

When I ran the integration test app and manually inspected it in the browser, I saw the uncaught JS exceptions displayed when running with the release version. The errors cleared up when I ran it using mvn -o to use my locally installed branch build.

I've never been able to get TestBench based integration tests in Vaadin projects running locally for me, so I'm shooting blind when committing test changes, relying on the CI build to run them, and then see what happens. Sometimes I can get some to run but not others, but that doesn't help me know if my changes work or not.

[WoozyG]

I was able to reproduce the grid based exceptions as well manually by adding the given code to one of the Pro starter apps. The keys to reproduce:

1. set the UI locale before attaching the TimePicker to the UI, using a locale with an easily identifiable display format. I use zn-CH because it's so different from Latin based formats, and hard to miss.

2. set an initial value, don't wait to set the value in a later response.

[alvarezguille]

I can help writing the test. First I would like to know how to get the error either in the overlay or console in a view like TimePickerInitializationView that you provide.

I undid the changes to TimePicker and tried to follow your suggestions and couldn't reproduce it.

I would like to avoid adding grid dependency even if it's only for integration tests, but if it's the only option it can be done.

[WoozyG]

@alvarezguille, I'm not sure what's going wrong for you. When I run the integration test locally with:

mvn -pl vaadin-time-picker-flow-integration-tests clean -o jetty:run

and a URL like:

http://localhost:9998/time-picker-initialization/zh-CN-13-50

to use my locally installed snapshot for the component, and removing the -o to let Maven override the local snapshot with the online version (no idea what voodoo Maven does to decide to ignore a locally installed snapshot, but it was).

However, my Chrome has updated twice in the last week, and now the tests, without changes on my part, behave differently. That's unusual, and I don't like unusual or unexplained. I'll keep looking into it as I have time.

[Haprog]

The issue (#22) has been fixed by #79. Closing this PR as unnecessary.