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

[consistency] locale version showing missing spaces #658

Closed
ghost opened this Issue Mar 3, 2016 · 7 comments

Comments

Projects
None yet
2 participants
@ghost

ghost commented Mar 3, 2016

A number of words are concateneted wihout the usual space in between in the locale column.

capture du 2016-03-03 20 36 19

@flodolo

This comment has been minimized.

Show comment
Hide comment
@flodolo

flodolo Mar 3, 2016

Contributor

I can confirm. The code is using Utils::secureText to output the text, I wonder if that's the issue (but it's weird such a different behavior between two strings so similar).

Contributor

flodolo commented Mar 3, 2016

I can confirm. The code is using Utils::secureText to output the text, I wonder if that's the issue (but it's weird such a different behavior between two strings so similar).

@pascalchevrel

This comment has been minimized.

Show comment
Hide comment
@pascalchevrel

pascalchevrel Mar 3, 2016

Member

The difference is that the first string has no line breaks while the second string has, my guess is that secureText removes the line breaks and doesn't replace them by spaces

Member

pascalchevrel commented Mar 3, 2016

The difference is that the first string has no line breaks while the second string has, my guess is that secureText removes the line breaks and doesn't replace them by spaces

@flodolo

This comment has been minimized.

Show comment
Hide comment
@flodolo

flodolo Mar 3, 2016

Contributor

Exactly, just realized that one version of the string doesn't have whitespaces but new lines
https://hg.mozilla.org/releases/l10n/mozilla-aurora/fr/file/default/dom/chrome/netError.dtd#l190

Not sure if we should change secureText or use a different method. I think secureText is used in several views.

Contributor

flodolo commented Mar 3, 2016

Exactly, just realized that one version of the string doesn't have whitespaces but new lines
https://hg.mozilla.org/releases/l10n/mozilla-aurora/fr/file/default/dom/chrome/netError.dtd#l190

Not sure if we should change secureText or use a different method. I think secureText is used in several views.

@pascalchevrel

This comment has been minimized.

Show comment
Hide comment
@pascalchevrel

pascalchevrel Mar 3, 2016

Member

I think it makes sense to add it to secureText because when we have a multiline string, we don't want the security filtering function to output a string that is different from the original one. Currently we remove ascii characters below 32 but we could just replace /n with a space before that operation.

Member

pascalchevrel commented Mar 3, 2016

I think it makes sense to add it to secureText because when we have a multiline string, we don't want the security filtering function to output a string that is different from the original one. Currently we remove ascii characters below 32 but we could just replace /n with a space before that operation.

@pascalchevrel pascalchevrel self-assigned this Mar 3, 2016

@flodolo

This comment has been minimized.

Show comment
Hide comment
@flodolo

flodolo Mar 3, 2016

Contributor

Agreed. We're probably broken in other views and never noticed it.

Contributor

flodolo commented Mar 3, 2016

Agreed. We're probably broken in other views and never noticed it.

@pascalchevrel

This comment has been minimized.

Show comment
Hide comment
@pascalchevrel

pascalchevrel Mar 3, 2016

Member

yes, I just checked the onestring view which uses secureText and it's broken for this string. Patch coming.

Member

pascalchevrel commented Mar 3, 2016

yes, I just checked the onestring view which uses secureText and it's broken for this string. Patch coming.

pascalchevrel added a commit to pascalchevrel/transvision that referenced this issue Mar 3, 2016

pascalchevrel added a commit to pascalchevrel/transvision that referenced this issue Mar 3, 2016

pascalchevrel added a commit that referenced this issue Mar 3, 2016

Merge pull request #660 from pascalchevrel/issue658_secureText_conver…
…t_newlines_to_spaces

Issue  #658: fix missing spaces in views
@pascalchevrel

This comment has been minimized.

Show comment
Hide comment
@pascalchevrel

pascalchevrel Mar 3, 2016

Member

fixed, thanks Goofy!

Member

pascalchevrel commented Mar 3, 2016

fixed, thanks Goofy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment