782779 - Using simple quotes for GroupItem's placeholder breaks some localized strings

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Théo Chevalier]

For the l10n of tabview.groupItem.defaultName (Name this group tab), for instance in French
 we have to use the character ' (Nommer ce groupe d'onglets)

So the end of the sentence is not displayed because of the quote.
I tried a localized build with (Nommer ce groupe d\'onglets), but it's the same.

I think we should use placeholder="" instead of placeholder='' since I guess no locale use the character " in their strings. (CC'ing Pike to be sure I'm right)

I upload a patch.

Comment 1

[Théo Chevalier]

Attachment: Patch

I tried this patch on a French localized build on my laptop, it worked.

Comment 2

[Axel Hecht [:Pike]]

Comment on attachment 651882 [details] [diff] [review]
Patch

Review of attachment 651882 [details] [diff] [review]:
-----------------------------------------------------------------

Let me chime in with an f-, this is bad code either way. This allows scripts tags to be inserted and whatnot. The localized content should either be escaped, or inserted post-hoc as plain text node.

Comment 3

[Théo Chevalier]

Comment on attachment 651882 [details] [diff] [review]
Patch

Thanks Pike, I'll address your comment.

Comment 4

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Attachment: patch

I had this patch in my tree that fixes the problem for me, I cleaned it up today, if that's not good enough maybe Theo can use it as a basis for a final fix.

I checked a few locales that I know use quotes regularly and that bug should also affect lij, br and cy, potentially others.

Comment 5

[Axel Hecht [:Pike]]

Comment on attachment 664894 [details] [diff] [review]
patch

Review of attachment 664894 [details] [diff] [review]:
-----------------------------------------------------------------

I'd rather use text-safe APIs than escaping and parsing.

::: browser/components/tabview/groupitems.js
@@ +98,5 @@
>      "</div>";
>  
>    this.$titlebar = iQ('<div>')
>      .addClass('titlebar')
>      .html(html)

I'd rather do iQ magic at this point here, and literally do
.attr('placeholder', this.defaultName)
or whatever's left of jqery in iQ.

Comment 6

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Attachment: updated patch using iQ() API

I didn't know about the sanitizing possibilities of the iQ library, here is an updated patch, it works on my locally compiled French build.

Comment 7

[Axel Hecht [:Pike]]

Comment on attachment 664928 [details] [diff] [review]
updated patch using iQ() API

Review of attachment 664928 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I like this better.

Comment 8

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Comment on attachment 664928 [details] [diff] [review]
updated patch using iQ() API

Tim, could you review this patch please?
Thanks

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 664928 [details] [diff] [review]
updated patch using iQ() API

Review of attachment 664928 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this!

r=me with the change below.

::: browser/components/tabview/groupitems.js
@@ +111,5 @@
>      .appendTo($container);
>  
>    // ___ Title
>    this.$titleContainer = iQ('.title-container', this.$titlebar);
>    this.$title = iQ('.name', this.$titlebar);

this.$title = iQ('.name', this.$titlebar).attr('placeholder', this.defaultName);

@@ +116,5 @@
>    this.$titleShield = iQ('.title-shield', this.$titlebar);
>    this.setTitle(options.title);
>  
> +  // ___ Input field
> +  this.$inputField = iQ('.name', this.$titlebar).attr('placeholder', this.defaultName);

We shouldn't need this extra property here and get the .name field again. We can just use the one above.

Comment 10

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Attachment: updated bug with changes asked

Here is the updated patch, I marked it as review+ (hope that's ok :) )

Should I add the checkin-needed tag in the keywords field to get that into mozilla-central?

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 664952 [details] [diff] [review]
updated bug with changes asked

Sorry, that patch doesn't contain the changes I asked for. Did you maybe attach the old version again?

Comment 12

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Attachment: updated patch, for real now !

oops, indeed I attached the wrong patch, sorry about that, let's try again :)

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 664973 [details] [diff] [review]
updated patch, for real now !

Review of attachment 664973 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks :)

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Pascal Chevrel:pascalc from comment #10)
> Should I add the checkin-needed tag in the keywords field to get that into
> mozilla-central?

That would be the normal procedure, yes. Let me just push it for you now :)

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/b56fa836e894

Comment 16

[Pascal Chevrel:pascalc (PTO until Sept 2)]

awesome, thanks Tim!

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/b56fa836e894