Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Translate directive whitespace handling can cause erroneous recursive translation #998

Closed
dpmott opened this issue Jan 31, 2019 · 8 comments

Comments

@dpmott
Copy link
Contributor

dpmott commented Jan 31, 2019

Current behavior

An HTML element with a translate directive and whose content is a translation key with leading/trailing spaces/newlines will return an erroneous recursive translation when the en.json file has an additional key matching the VALUE of the referenced translation key.

Example HTML:

<div translate>
  NAV_BAR.LINK_TEXT
</div>

Example en.json:

  "NAV_BAR": {
    "LINK_TEXT": "EXAMPLE"
  },
  "EXAMPLE": {
    "THIS": "this",
    "THAT": "that"
  }

With this example setup, the <div> will display [object Object].

Note that the following HTML will not trigger the bug:
<div translate>NAV_BAR.LINK_TEXT</div>

Expected behavior

The <div> should display EXAMPLE.

How do you think that we should fix this?

On this line: https://github.com/ngx-translate/core/blob/master/projects/ngx-translate/core/src/lib/translate.directive.ts#L82

Change if (content!== node.currentValue)
to
if (trimmedContent !== node.currentValue)
because
https://github.com/ngx-translate/core/blob/master/projects/ngx-translate/core/src/lib/translate.directive.ts#L115
explicitly preserves whitespace in the node content, so only the trimmed content must be treated as the key.

Minimal reproduction of the problem with instructions

https://github-ghfrtw.stackblitz.io

Environment


ngx-translate version: 10.0.2
Angular version: 6.1.10

This is still an issue in the latest ngx-translate version.
This was not an issue in ngx-translate 8.0.0

Browser:
- [X ] Chrome (desktop) Version 71.0.3578.98 (Official Build) (64-bit)
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX

(Presumably other browsers as well; I didn't check them.)
 
@BADF00D
Copy link
Contributor

BADF00D commented Feb 13, 2019

I can confirm that this bug exists. I have a similar working example. In my version, the initial binding does work just fine (event with line breaks), but when I change the language at runtime, these values doesn't get updated.

Render Stackblitz
Editor Stackblitz

I also can confirm, that this error is browser independent. Looks the same in Edge (41) and Firefox (64).

@dpmott When you add this missing translation to the fr.json, your initial binding will be correctly show your English translation (that sounds weird, but it works), but when you change the translations at runtime (via translate.use('fr')) the translations with whitespaces wont be updated. You can refer to my example.

@BADF00D
Copy link
Contributor

BADF00D commented Feb 13, 2019

I can also confirm, that the error is fixed, when applying the fix mentioned by @dpmott

@dpmott
Copy link
Contributor Author

dpmott commented Feb 13, 2019

@badfood Thanks for your thorough investigation.

Do you need a PR from me?

@BADF00D
Copy link
Contributor

BADF00D commented Feb 13, 2019

@dpmott I'm no maintainer of the project. I'm just a guy that stumbles upon the same error.

But feel free to create a PR, I did it myself but was not yet able to fix the failing test: × should update the DOM when the lang changes and the translation ends with space

@BADF00D
Copy link
Contributor

BADF00D commented Feb 19, 2019

@dpmott Finally I was able to create a PR with no failing tests. But I don't think there will be an bug fix release for 10.X, so I think we have to update to latest angular.

@BADF00D
Copy link
Contributor

BADF00D commented Mar 1, 2019

As long as the Pull-Request is not merged, I have use the translate pipe, instead of the translate directive

Instead of using this

<div translate>
   MyText
</div>

becomes

<div >
   {{'MyText' | translate}}
</div>

If you have to translate enums or something else with prefixes, I use this:
Instead of

<div translate>RunState.{{ details?.infusion.status }}</div>
<div>{{ details?.infusion.status | prefix:'RunState.' | translate}}</div>

The prefix pipe is implemented as:

import { Pipe, PipeTransform } from '@angular/core';

@Pipe({
  name: 'prefix',
  pure: true
})
export class PrefixPipe implements PipeTransform {
  transform(value: any, args?: any): any {
    const msg = <string>value;
    if (value == null) {
      return null;
    }

    return `${args}${msg}`;
  }
}

dpmott added a commit to dpmott/core that referenced this issue Nov 15, 2019
ocombe pushed a commit to dpmott/core that referenced this issue Feb 5, 2020
@ocombe ocombe closed this as completed in 35427b0 Feb 5, 2020
@Crylion
Copy link

Crylion commented Feb 7, 2020

When using 12.0.0 with Angular 9 the issue really just got worse for me instead of better.
Before the update the translations via the directive worked initially and just failed to update on language change, now it doesn't work at all when there is a line break.

So like in the example by BADF00D

<div translate>
   MyText
</div>

this would work initially in 11.0.0, but would not update correctly on language change, now it does not work at all anymore...
Only removing the whitespace completely fixes it now (or using the pipe of course)

@dpmott
Copy link
Contributor Author

dpmott commented Feb 10, 2020

In the angular apps that I've been writing, I haven't included a dynamic option to change the language. However, the comment from @Crylion above caught my attention, and I've been able to reproduce the described behavior.

I'll file a new issue (as it doesn't seem that a new issue has been created yet), and I'll investigate a fix that will work for everyone.

Update: Opened #1163

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants