escape ' -> \' for the UI-WELCOME_ERROR #10581

srl295 · 2017-02-25T01:24:56Z

add a new function i18n_singleQuoteVar() for ui_app.jade so that “err.innerText = '#{i18n_singleQuoteVar('UI-WELCOME_ERROR')}';” doesn’t fail if the UI-WELCOME_ERROR contains a single quote

Fixes: #10580

elasticmachine · 2017-02-25T01:24:58Z

Can one of the admins verify this patch?

srl295 · 2017-02-25T01:25:16Z

FYI @hickeyma

epixa · 2017-02-26T14:33:13Z

jenkins, test this

hickeyma · 2017-02-27T10:54:13Z

src/ui/views/ui_app.jade

@@ -125,7 +125,7 @@ block content
          err.style['text-align'] = 'center';
          err.style['background'] = '#F44336';
          err.style['padding'] = '25px';
-          err.innerText = '#{i18n('UI-WELCOME_ERROR')}';
+          err.innerText = '#{i18n_singleQuoteVar('UI-WELCOME_ERROR')}';



I am wondering would it be better here to assign to double quotes instead to escape the single quote:
err.innerText = "#{i18n('UI-WELCOME_ERROR')}";

@hickeyma I tried that, actually using JSON.stringify instead and double quotes… but Jade (now pug) ends up quoting oddly- I think a html safe transform is implicated.

still would need to quote doublequotes in that case.

for that matter, this line could just be:

`err.innerText = '#{i18n('UI-WELCOME_ERROR').replace(/'/g,'\'')}';

and drop the other function.

I tried this and it works as expected without any other cahnges:
err.innerText = "#{i18n('UI-WELCOME_ERROR')}";

will that work if a " shows up in the string? In Hebrew that can be used as a standin for gershayim ״ (in Hebrew). So it might fix French but break Hebrew.

what about something like err.innerText = "#{i18n('UI-WELCOME_ERROR').replace(/"/g,'\\"')}"; for this one spot (if we don't use the meta tag option)

you are in a maze of twisty nested quoted strings, some alike

hickeyma · 2017-02-27T10:55:55Z

src/ui/index.js

        i18n: key => _.get(translations, key, ''),
+        // var v = #{i18n_var('KEY')} -> var v = "value (\"Hi!\")"
+        i18n_singleQuoteVar: key => _.get(translations, key, '').replace(/'/g,'\\\''),
      });


Not sure we would want 2 variables passed which contain the separate instances of the translations. Refer to comment in 'src/ui/views/ui_app.jade' by using double quotes.

they're two functions… and actually, they are only for this startup page.

Ahhhh .. meant functions! Just trying to avoid having to make changes if another means possible.

hickeyma

@srl295 Added some feedback. I would prefer to avoid to have to explicitly escape and possibly handle the string more effective in jade.

spalger · 2017-02-27T16:43:20Z

Take notice how we handle this here: https://github.com/srl295/kibana/blob/559bb8bf3cbad7a189681f9ae864fc4f9f1cbe0c/src/ui/views/ui_app.jade#L104

srl295 · 2017-02-27T17:07:43Z

@spalger I thought about that, but was not sure how to stringify the existing function. !{JSON.stringify(#{i18n(…)})} I didn't think would work becasue #{ would be interpreted in JS, but perhaps that's not true.

spalger · 2017-02-27T21:45:39Z

@srl295 the JSON stringification happens server side, so the line would look like:

err.innerText = !{JSON.stringify(i18n(…))};

Now that I think about it, that wouldn't work if the string had html in it 😞 . Escaping in a script tag is hard (which is why we ended up using a meta tag for our injected vars).

epixa · 2017-02-27T22:21:52Z

Can we just use a meta tag for this?

srl295 · 2017-03-01T01:29:43Z

@epixa @spalger yeah— we could stash the message off somewhere… and then go find it later. Then Jade^H^H^H^Hpug handles the html safe escaping.

hickeyma · 2017-03-01T15:33:17Z

@epixa, @spalger : would this not work?
err.innerText = "#{i18n('UI-WELCOME_ERROR')}";

spalger · 2017-03-01T16:16:40Z

@hickeyma it would probably work, but I'm just not comfortable writing an escaping function inline (or elsewhere) unless it's tested and the requirements of the function are better researched/more certain.

srl295 · 2017-03-01T19:16:38Z

@spalger let me try the meta tag option.

srl295 · 2017-03-08T00:00:07Z

@epixa @spalger @hickeyma hey, enjoy your conf! And FYI, new commit using a div (keeps the messages together)

srl295 · 2017-03-21T01:44:58Z

I've rebased now that #8766 is merged.

epixa · 2017-03-21T15:21:57Z

jenkins, test this

hickeyma

LGTM. @spalger and @Bargs, what do you think?

tsullivan · 2018-02-21T20:09:43Z

looks like there is a conflict in this PR. Does src/ui/views/ui_app.jade need to be src/ui/ui_render/views/ui_app.jade ?

LGTM. @spalger and @Bargs, what do you think?

Ping @spalger and @Bargs

elasticmachine · 2018-02-21T20:09:45Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2018-02-21T20:37:10Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

srl295 · 2018-02-21T21:04:17Z

Hey happy new year and all! I think this may need some serious rebasing.

@tsullivan as you noted the changed file in question is now at https://github.com/elastic/kibana/blob/master/src/ui/ui_render/views/ui_app.jade - it was moved in #14745 and modified in #15904, it's possible this change is moot.

srl295 · 2018-02-21T21:28:11Z

Old commit was srl295@c17baa4 — pushing an untested rebase now.

Bargs · 2018-02-21T21:37:17Z

Not sure if this is still relevant or not, Spencer would probably know best.

srl295 · 2018-02-21T22:19:43Z

I couldn't remember how I tested this. I'm doing this locally to test:

diff --git a/src/core_plugins/kibana/translations/en.json b/src/core_plugins/kibana/translations/en.json
index ddb7a272e..37e7553a2 100644
--- a/src/core_plugins/kibana/translations/en.json
+++ b/src/core_plugins/kibana/translations/en.json
@@ -1,4 +1,4 @@
 {
   "UI-WELCOME_MESSAGE": "Loading Kibana",
-  "UI-WELCOME_ERROR": "Kibana did not load properly. Check the server output for more information."
+  "UI-WELCOME_ERROR": "Kibana didn't load properly. Check the server output for more information."
 }
diff --git a/src/ui/ui_render/bootstrap/template.js.hbs b/src/ui/ui_render/bootstrap/template.js.hbs
index 8a4c5fe52..6ed394f6e 100644
--- a/src/ui/ui_render/bootstrap/template.js.hbs
+++ b/src/ui/ui_render/bootstrap/template.js.hbs
@@ -40,6 +40,8 @@ window.onload = function () {
     dom.setAttribute('async', '');
     dom.addEventListener('error', failure);

+    failure();
+
     if (type === 'script') {
       dom.setAttribute('src', file);
       dom.addEventListener('load', next);

srl295 · 2018-02-21T22:20:07Z

@Bargs Hi! I was able to get a rebase to work. I'm checking to see if I can repro the original issue.

srl295 · 2018-02-21T22:21:43Z

Yup I can repro.

see my above comment, fails (spins forever on loading) because of:

err.innerText = 'Kibana didn't load properly. Check the server output for more information.';

So, it's still relevant. And the PR fixes it. Rerun it through CI?

Bargs · 2018-02-21T22:57:21Z

jenkins, test this

elasticmachine · 2018-02-21T23:47:04Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

Bargs · 2018-03-27T19:31:15Z

I didn't test the fix but the code looks ok to me. I'm going to assign @azasypkin as a reviewer as he's taking over the internationalization effort, so he can decide if it makes sense to merge this or not.

azasypkin

Hey @srl295, sorry for the huge delay! It slipped under my radar somehow.

PR looks good to me, just one nit and one question. Also please rebase your PR on the latest master to make sure CI is green? Thanks!

azasypkin · 2018-05-03T09:52:15Z

src/ui/ui_render/bootstrap/template.js.hbs

      var err = document.createElement('h1');
      err.style['color'] = 'white';
      err.style['font-family'] = 'monospace';
      err.style['text-align'] = 'center';
      err.style['background'] = '#F44336';
      err.style['padding'] = '25px';
-      err.innerText = '{{i18n 'UI-WELCOME_ERROR'}}';
+      err.innerText = failureText;


nit: since we don't use i18n helper anymore we can safely remove it from app_bootstrap.js (including all other related pieces like lodash import and knownHelpers property).

@azasypkin hm - well, the point here is to load the localized error. What's the right way to do it now?

The entire bug here is that at runtime,

err.innerText = '{{i18n 'UI-WELCOME_ERROR'}}';

could (with the right translation) get replaced by the i18n helper to:

err.innerText = 'Quel temps va-t-il faire aujourd'hui?';

… which is invalid JS.

Right, I mean previously we rendered message with the help of i18n Handlebars helper (see here) that led to invalid JS. In this PR you're extracting error message from the DOM, so we don't need this helper anymore.

i18n Jade helper used in ui_app.jade or chrome.jade (see here is a different thing, we still need it.

got it. now when i get a chance I'll rebase and update.

@azasypkin Done, please re review?

azasypkin · 2018-05-03T11:14:13Z

src/ui/ui_render/views/ui_app.jade

@@ -109,5 +113,7 @@ block content
        .kibanaWelcomeLogo
    .kibanaWelcomeText
      | #{i18n('UI-WELCOME_MESSAGE')}
+    #kibanaLoader__error


suggestion: what if we use data attribute instead?

.kibanaWelcomeText(data-error-message='#{i18n("UI-WELCOME_ERROR")}')

It won't require additional style with display: none, in js we can extract it like this:

document.querySelector('[data-error-message]').dataset.errorMessage

Or even maybe add this attribute to body in chrome.jade and then we won't need querySelector at all:

document.body.dataset.errorMessage

What do you think?

@azasypkin makes some sense. But still uses the i18n helper?

But still uses the i18n helper?

Yes, but see my comment above, we need Jade i18n helper, not the one we used in Handlebars template.

Sorry, it's confusing, I know :)

markov00 · 2018-10-10T15:08:13Z

Hey @srl295 Thanks for your contribution, the i18n implementation has changed in the last months, do you have time to rebase to master?

* store the error message in an extra div for later retrieval Fixes: elastic#10580

elasticmachine · 2018-10-10T18:10:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

srl295 · 2018-10-10T18:37:56Z

@markov00 I have addressed @azasypkin ’s suggestions and rebased. However, I can't figure out how to 'break' Kibana in order to get the error message to show up now. Do you have any suggestions?

markov00 · 2018-10-13T10:36:23Z

Jenkins, test this

@srl295 thanks for that 👍
@azasypkin have you any suggestion on how to test this? Do we already have testing suite for this cases?

azasypkin · 2018-10-13T11:43:13Z

src/ui/ui_render/views/ui_app.pug

@@ -107,7 +107,7 @@ block content
      .kibanaLoader
      .kibanaWelcomeLogoCircle
        .kibanaWelcomeLogo
-    .kibanaWelcomeText
+    .kibanaWelcomeText(data-error-message='#{i18n("UI-WELCOME_ERROR")}')


issue: that's why CI is failing (https://kibana-ci.elastic.co/job/elastic+kibana+pull-request+multijob-intake/4504/console):

12:49:04 [10:49:04] → I18N ERROR Error in src/ui/ui_render/views/ui_app.pug 12:49:04 Error: Empty defaultMessage in i18n() or i18n.translate() is not allowed ("UI-WELCOME_ERROR"). 12:49:04 ERROR I18N ERROR Error in src/ui/ui_render/views/ui_app.pug 12:49:04 Error: Empty defaultMessage in i18n() or i18n.translate() is not allowed ("UI-WELCOME_ERROR").

I guess you wanted to write something like this instead

.kibanaWelcomeText(data-error-message="#{i18n('common.ui.welcomeError', { defaultMessage: 'Kibana did not load properly. Check the server output for more information.' })}")`

Our tools now enforce default message for all labels that allows developers to actually see the real label content without looking into translation files. So we got rid of en.json entirely and extract all default messages automatically whenever translation vendors need them.

@pavel06081991 @maryia-lapata can you please help @srl295 to test this change (i.e where one should put and register test translation file for common.ui. etc.)? Essentially we need a localized version of this label that includes apostrophe.

@srl295
As far as I understood, the code below doesn't really call function i18n

.kibanaWelcomeText(data-error-message="#{i18n('common.ui.welcomeErrorMessage', { defaultMessage: 'Kibana did not load properly. Check the server output for more information.' })}")

And the attribute value is displayed as it is:

If we change code to invoke i18n, like this:

.kibanaWelcomeText(data-error-message=i18n('common.ui.welcomeErrorMessage', { defaultMessage: 'Kibana did not load properly. Check the server output for more information.' }))

it will be rendered correctly (I checked with fr locale):

But i18n tool doesn't extract such message for now. @LeanidShutau just created PR #24006 to fix this.

In case you would like to check translations with custom locale, here the steps are:

run command node scripts/i18n_check --output ./. It will generate en.json file with default translations and it place the file in the root folder.

create fr.json file, copy translations from en.json, update messages in fr.json according to locale (you can just update one or several messages).

point plugin to the translation file. For example, in src/core_plugins/kibana/index.js add path to fr.json file in translations array:

translations: [ require('path').resolve(__dirname, './../../../fr.json') ],

set up locale in config/kibana.yml by changing i18n.locale param. (i18n.locale: "fr")

Hey @srl295! Please let us know if you need any help with this PR. We intend to ship fix for this issue in the 6.6 release, but If you don't have time that's fine too! Someone from the i18n team will just take it over.

elasticmachine · 2018-10-13T12:22:01Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

azasypkin · 2019-02-01T20:47:32Z

Hey @srl295, it looks like you were busy with other stuff lately, but no worries! Since we wanted to have the fix for this issue in 6.7 we moved forward and fixed it in #29581 (basically a bit cleaned up version of your PR).

Thank you for your contribution!

epixa added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc review labels Feb 26, 2017

epixa assigned spalger Feb 26, 2017

hickeyma reviewed Feb 27, 2017

View reviewed changes

srl295 force-pushed the fix-broken-quote branch from 559bb8b to 6c30eb7 Compare March 7, 2017 23:59

srl295 force-pushed the fix-broken-quote branch from 6c30eb7 to c17baa4 Compare March 21, 2017 01:44

hickeyma approved these changes Apr 9, 2017

View reviewed changes

srl295 force-pushed the fix-broken-quote branch from c17baa4 to 246e7b9 Compare February 21, 2018 21:30

Bargs self-requested a review March 22, 2018 22:08

Bargs requested review from azasypkin and removed request for Bargs March 27, 2018 19:31

tsullivan assigned azasypkin May 1, 2018

azasypkin requested changes May 3, 2018

View reviewed changes

azasypkin added the Project:i18n label Aug 24, 2018

Move UI-WELCOME_ERROR from a script insertion to a hidden <div>

c640e69

* store the error message in an extra div for later retrieval Fixes: elastic#10580

srl295 force-pushed the fix-broken-quote branch from 246e7b9 to c640e69 Compare October 10, 2018 18:36

azasypkin reviewed Oct 13, 2018

View reviewed changes

azasypkin added the v6.6.0 label Nov 2, 2018

azasypkin unassigned spalger Nov 2, 2018

maryia-lapata added v6.7.0 and removed v6.6.0 labels Dec 18, 2018

maryia-lapata mentioned this pull request Jan 30, 2019

[i18n] Move welcome error message from a script insertion to a data attribute #29581

Merged

azasypkin closed this Feb 1, 2019

escape ' -> \' for the UI-WELCOME_ERROR #10581

escape ' -> \' for the UI-WELCOME_ERROR #10581

Conversation

srl295 commented Feb 25, 2017

elasticmachine commented Feb 25, 2017

srl295 commented Feb 25, 2017

epixa commented Feb 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srl295 Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hickeyma left a comment

Choose a reason for hiding this comment

spalger commented Feb 27, 2017

srl295 commented Feb 27, 2017

spalger commented Feb 27, 2017

epixa commented Feb 27, 2017

srl295 commented Mar 1, 2017

hickeyma commented Mar 1, 2017

spalger commented Mar 1, 2017

srl295 commented Mar 1, 2017

srl295 commented Mar 8, 2017 • edited Loading

srl295 commented Mar 21, 2017

epixa commented Mar 21, 2017

hickeyma left a comment

Choose a reason for hiding this comment

tsullivan commented Feb 21, 2018

elasticmachine commented Feb 21, 2018

elasticmachine commented Feb 21, 2018

💔 Build Failed

srl295 commented Feb 21, 2018 • edited Loading

srl295 commented Feb 21, 2018 • edited Loading

Bargs commented Feb 21, 2018

srl295 commented Feb 21, 2018

srl295 commented Feb 21, 2018

srl295 commented Feb 21, 2018

Bargs commented Feb 21, 2018

elasticmachine commented Feb 21, 2018

💚 Build Succeeded

Bargs commented Mar 27, 2018

azasypkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin May 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markov00 commented Oct 10, 2018 • edited Loading

elasticmachine commented Oct 10, 2018

💔 Build Failed

srl295 commented Oct 10, 2018

markov00 commented Oct 13, 2018

Choose a reason for hiding this comment

maryia-lapata Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 13, 2018

💔 Build Failed

azasypkin commented Feb 1, 2019

srl295 Mar 1, 2017 •

edited

Loading

srl295 commented Mar 8, 2017 •

edited

Loading

srl295 commented Feb 21, 2018 •

edited

Loading

srl295 commented Feb 21, 2018 •

edited

Loading

azasypkin May 3, 2018 •

edited

Loading

markov00 commented Oct 10, 2018 •

edited

Loading

maryia-lapata Oct 15, 2018 •

edited

Loading