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

Trans renders tags in interpolation values as react elements #919

Closed
radiansz opened this issue Aug 13, 2019 · 10 comments
Closed

Trans renders tags in interpolation values as react elements #919

radiansz opened this issue Aug 13, 2019 · 10 comments

Comments

@radiansz
Copy link

@radiansz radiansz commented Aug 13, 2019

I'm not sure if it's a bug or expected behavior, didnt find definitive answer in docs and issues.

Describe the bug
Trans renders tags in interpolation values as react elements instead of strings(if flag transSupportBasicHtmlNodes is set to true of course)

If transSupportBasicHtmlNodes is set to false, value with html tags fails to render as string also, and react elements matching tags is not rendered.

Occurs in react-i18next version
10.11.5

To Reproduce
Reproduction based on example from docs: https://codesandbox.io/s/react-i18next-qlwhe?fontsize=14

To check behavior with transSupportBasicHtmlNodes: false, change it's value in i18n.js

Expected behaviour
Tags in interpolation values are being ignored and rendered as strings with transSupportBasicHtmlNodes set to either true or false

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Aug 13, 2019

if set true it's expected for: https://github.com/i18next/react-i18next/blob/master/src/context.js#L9 that replace in interpolation happens before converting to react-elements

@radiansz

This comment has been minimized.

Copy link
Author

@radiansz radiansz commented Aug 13, 2019

If this parameter designed to describe tags in translation strings to replace with react elements, then it's not working right since in codesandbox above it renders video tag, and it is not in defaults.
I thought this parameter is not for translation strings but for children of Trans

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Aug 13, 2019

every child element in trans will be set if there is a <1> or whatever index there i - the above only applies to tags inside the translation string (like
not in Trans children)

@radiansz

This comment has been minimized.

Copy link
Author

@radiansz radiansz commented Aug 13, 2019

Yes. But problem i am trying to describe is not connected with <0></0> notation. It's about what we pass in values prop and how Trans handles it inside.

Sorry if my explanation is vague, or problem trivial, but i do not see sane workaround for now

I'll try to explain again:

If i do(with transSupportBasicHtmlNodes: true)

resources: {
  something: {
    foo: 'Something "{{bar}}"'
  }
}
-----
<Trans i18nKey="foo" values={{ bar: '<video/>' }} />

it renders as

Something "[[actual dom video element]]"

And i can live with this, tho for me it's hard to find use case for it

Next i disable transSupportBasicHtmlNodes

And do the same(with transSupportBasicHtmlNodes: false)

resources: {
  something: {
    foo: 'Something "{{bar}}"'
  }
}
-----
<Trans i18nKey="foo" values={{ bar: '<video/>' }} />

it renders as

Something ""

but i expecting

Something "<video/>"

And because of that it's impossible to pass any user input in Transs values.

So to implement message like this,

search: Elements matching <b>"{{query}}"</b> havent been found

And keeping in mind that i cant pass any user input in values, i'd have to do this

<div>
  {t('search_part1')}
  <b>{query}</b>
  {t('search_part2')}
</div>
@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Aug 13, 2019

Can you retry with react-i18next@10.12.2 it should check for https://github.com/i18next/react-i18next/blob/master/src/context.js#L9 allowed tags

@radiansz

This comment has been minimized.

Copy link
Author

@radiansz radiansz commented Aug 14, 2019

Yup that made things better. Sorry i havent figured out version bump myself, for some reason i thought i using the last one, will try to avoid that mistake in the future.

Unfortunately there is still some unexpected behaviour

transSupportBasicHtmlNodes: true,
transKeepBasicHtmlNodesFor: ["br", "strong", "i", "p"],
-----
translations: {
  search: "Elements matching <b>'{{query}}'</b> havent been found",
}
-----
<Trans i18nKey="search" values={{ query: '<i>test<video/></i>' }} />

renders as

Elements matching <b>',[object Object],'</b> havent been found

Seems like [object Object] happens when a tag from transKeepBasicHtmlNodesFor is inside a tag that is not in transKeepBasicHtmlNodesFor


We can simplify

transSupportBasicHtmlNodes: true,
transKeepBasicHtmlNodesFor: ["br", "strong", "i", "p"],
-----
translations: {
  search: "<b><i></i></b>",
}
-----
<Trans i18nKey="search"/>

=>

<b>[object Object]</b>


also if we do

transSupportBasicHtmlNodes: false,
transKeepBasicHtmlNodesFor: ["br", "strong", "i", "p"],
-----
translations: {
  search: "Elements matching <b>'{{query}}'</b> havent been found",
}
-----
<Trans i18nKey="search" values={{ query: '<i>test<video/></i>' }} />

it renders as

Elements matching <b>',<i>test,<video /></i>,'</b> havent been found

With unwanted commas at the start and the end, and between test and <video/>

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Aug 14, 2019

The first case: Elements matching <b>',[object Object],'</b> havent been found

I assume b tag is rendered as string (as the code should do) -> still the inner Elements get transformed returning node children -> node children toString --> [object Object]

While [object Object] is not nice - still the translation string is "wrong" for the settings you got (not allowing b tag).

The second case: Elements matching <b>',<i>test,<video /></i>,'</b> haven't been found the code does not know what to do with that AST node - it is not allowed handling it as transKeepBasicHtmlNodesFor it's also not a valid node given in react, and it's not an object to interpolate...so it ends in some case (my guess) https://github.com/i18next/react-i18next/blob/master/src/Trans.js#L168 and the end result is something odd

  • interpolation does not escape values (like in i18next) as react already escapes them => would result in double escaping.
  • we need to map the translation string to an html AST for Trans to work and can't differ between the ones from the translation string and the ones from interpolation easily.

Currently, at least this is fixed regarding security concerns...not fixed is interpolating tags into content - but honestly, I'm not sure we can support that...won't be easy.

Do you really need to interpolate <Trans i18nKey="search" values={{ query: '<i>test<video/></i>' }} />?!?

@radiansz

This comment has been minimized.

Copy link
Author

@radiansz radiansz commented Aug 14, 2019

First of all, i solved my use case(displaying user input bold in translation string) via updating react-i18next(as you suggested) and doing:

transSupportBasicHtmlNodes: false,
-----
search: `It's bold user input <0>{{userInput}}</0> i'm showing you`,
-----
<Trans i18nKey="search" values={{ userInput }} components={[<b>whatever</b>]} />

it's works relatively fine for me

I reported additional bugs just for the sake of informing about their existance, since i came across them. I mean, i can live with them fine.

Some thoughts about your what you said:

still the translation string is "wrong" for the settings you got

I can think of use case when you want to show some html tags as string. For example some html tutorial/example or whatever. So i'm not sure if it is neceserily wrong. On the other hand, you'd probably want transSupportBasicHtmlNodes turned off for that use case.

Currently, at least this is fixed regarding security concerns...not fixed is interpolating tags into content - but honestly, I'm not sure we can support that...won't be easy.

Yeah i understand. I can try to fix it and send PR if attempt end up successful. If it's fine of course

Do you really need to interpolate <Trans i18nKey="search" values={{ query: 'test' }} />?!?

I really dont want to do such things :). But it bothers me that users can put some wild interpolation value here.

Anyway thank you for quick responses, help and your work on the such a great lib!

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Aug 14, 2019

Sure PR's are always welcome...I will look into it too as soon I got more time - yet not absolutely sure how to approach it...(escaping interpolation and undo again after transforming to react or just adding a different handler for those cases processing the AST children to string)

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Sep 17, 2019

closing this for now....looking forward to an eventual PR

@jamuhl jamuhl closed this Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.