Last Comment Bug 782779 - Using simple quotes for GroupItem's placeholder breaks some localized strings
: Using simple quotes for GroupItem's placeholder breaks some localized strings
Status: RESOLVED FIXED
[mozfr]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: unspecified
: All All
: -- trivial
: Firefox 18
Assigned To: Pascal Chevrel:pascalc
:
:
Mentors:
Depends on:
Blocks: 781626
  Show dependency treegraph
 
Reported: 2012-08-14 14:11 PDT by Théo Chevalier [:tchevalier]
Modified: 2016-04-12 14:00 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (1.09 KB, patch)
2012-08-14 14:14 PDT, Théo Chevalier [:tchevalier]
l10n: feedback-
Details | Diff | Splinter Review
patch (1.91 KB, patch)
2012-09-26 04:47 PDT, Pascal Chevrel:pascalc
l10n: feedback-
Details | Diff | Splinter Review
updated patch using iQ() API (1.59 KB, patch)
2012-09-26 06:21 PDT, Pascal Chevrel:pascalc
ttaubert: review+
Details | Diff | Splinter Review
updated bug with changes asked (1.59 KB, patch)
2012-09-26 07:30 PDT, Pascal Chevrel:pascalc
ttaubert: review-
Details | Diff | Splinter Review
updated patch, for real now ! (1.58 KB, patch)
2012-09-26 08:03 PDT, Pascal Chevrel:pascalc
ttaubert: review+
Details | Diff | Splinter Review

Description Théo Chevalier [:tchevalier] 2012-08-14 14:11:31 PDT
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 [:tchevalier] 2012-08-14 14:14:15 PDT
Created attachment 651882 [details] [diff] [review]
Patch

I tried this patch on a French localized build on my laptop, it worked.
Comment 2 Axel Hecht [:Pike] 2012-08-14 14:18:25 PDT
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 [:tchevalier] 2012-08-14 14:31:06 PDT
Comment on attachment 651882 [details] [diff] [review]
Patch

Thanks Pike, I'll address your comment.
Comment 4 Pascal Chevrel:pascalc 2012-09-26 04:47:15 PDT
Created attachment 664894 [details] [diff] [review]
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] 2012-09-26 04:50:18 PDT
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 2012-09-26 06:21:16 PDT
Created attachment 664928 [details] [diff] [review]
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] 2012-09-26 06:35:22 PDT
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 2012-09-26 06:48:22 PDT
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] 2012-09-26 07:11:55 PDT
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 2012-09-26 07:30:06 PDT
Created attachment 664952 [details] [diff] [review]
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] 2012-09-26 07:32:22 PDT
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 2012-09-26 08:03:56 PDT
Created attachment 664973 [details] [diff] [review]
updated patch, for real now !

oops, indeed I attached the wrong patch, sorry about that, let's try again :)
Comment 13 Tim Taubert [:ttaubert] 2012-09-26 08:05:19 PDT
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] 2012-09-26 08:06:14 PDT
(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] 2012-09-26 08:08:22 PDT
https://hg.mozilla.org/integration/fx-team/rev/b56fa836e894
Comment 16 Pascal Chevrel:pascalc 2012-09-26 08:10:39 PDT
awesome, thanks Tim!
Comment 17 Tim Taubert [:ttaubert] 2012-09-27 00:13:54 PDT
https://hg.mozilla.org/mozilla-central/rev/b56fa836e894

Note You need to log in before you can comment on or make changes to this bug.