translateCloak directive adds cloak class but does not remove it

[chrisscholly]

Hi guys,

As you know, translateCloak directive adds cloak class and remove it when '$translateLoadingSuccess' event get fired.

But '$translateLoadingSuccess' is not always fired. In that case cloak class will never been removed.

Explanations

When translation tables have been loaded asynchronously (loadAsync function), '$translateLoadingSuccess' is fired and then useLanguage() is called with the right language key.

After the next use() call with the same language key, translation tables are no longer loaded async. because there is a translation table for the language we've requested. '$translateLoadingSuccess' has no chance to get fired in that case.

Solution

As in both cases useLanguage() is called and '$translateChangeEnd' is fired, we could listen to '$translateChangeEnd' rather than '$translateLoadingSuccess' in translateCloak directive to remove the cloak class.

If it makes sense sure I'll make a pull request.
Chris

[tspaeth]

Would you mind setting up an example?
I first thought "wow, yes", but then I played around with our tutorials and a bit of ngRoute but couldn't find a situation where the translate-cloak class is not removed.
When is the useLanguage not fired because at least in the current canary version I'll always - even with the async callers - come to the useLanguage - function?!

[chrisscholly]

Here's a plunker that illustrates the issue: http://plnkr.co/edit/UJXvdW
Use this url only to see how it works (because of url tricks). Use the urls below to run the example.

To reproduce the bug:

1. http://run.plnkr.co/plunks/UJXvdW/#/en_US
   => 'Hello there!'
2. Replace en_US by de_DE: http://run.plnkr.co/plunks/UJXvdW/#/de_DE
   => 'Hello da!'
3. Replace back de_DE by en_US: http://run.plnkr.co/plunks/UJXvdW/#/en_US
   => stays at "LOADING..." (while 'Hello there!' should appear, as step 1).
   This is because cloak class is not removed at step 3.

If we listen to '$translateChangeEnd' instead of '$translateLoadingSuccess' in translateCloak directive, it will works as well and 'Hello there!' will display at step 3.

[knalli]

I've looked into it.

As a side note:

1. I'm actually considering to change the event. $translateLoadingSuccess is actually emitted before the translation table is filled locally.. that's at least semantically not good (here: better $translateLoadingEnd).
2. The cloak will not work if there is no custom loader defined. The existence of a cloak would be a challenge, but a customer loader is also only a configuration part. Even so the cloak should be removed IMHO. Option: Use $translateChangeSuccess (better $translateChangeEnd) which also solves point 1.

But coming to your issue: I also do not get it. In addition, your test case 1.2 works for me. Hello there! is printed.

[0x-r4bbit]

@knalli @tspaeth I think @chrisscholly explained pretty well what the issue is. Removing translate-cloak class only when $translateChangeSuccess event is fired, is actually the wrong behaviour. Just imagine the simple case where $translateChangeError is fired. In that case the class will not be removed but should actually, because even if succeeded or not, the loader finished and therefore the class should be removed.

@chrisscholly If you're fancy making a pull request that makes that chance, just go for it.

[knalli]

Have a look at https://github.com/angular-translate/angular-translate/tree/bugfix/658, especially 3397e1f

Well, however, this would be invoked always. Also on error cases.

[0x-r4bbit]

@knalli that's perfect. I'll cherry pick it on top of latest canary

[0x-r4bbit]

Landed as acab18a will land with the next release, thank you @chrisscholly for submitting and @knalli for implementing. You guys rock.

[chrisscholly]

Thank you @knalli for implementing :) Can't wait to use the next release of angular-translate ! :)