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

RESOLVED FIXED in Firefox 18

Status

Firefox Graveyard
Panorama
--
trivial
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: tchevalier, Assigned: pascalc)

Tracking

unspecified
Firefox 18

Details

(Whiteboard: [mozfr])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 651882 [details] [diff] [review]
Patch

I tried this patch on a French localized build on my laptop, it worked.
Assignee: nobody → theo.chevalier11
Status: NEW → ASSIGNED
Attachment #651882 - Flags: review?(ttaubert)
Assignee: theo.chevalier11 → nobody
Status: ASSIGNED → NEW
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.
Attachment #651882 - Flags: feedback-
(Reporter)

Comment 3

5 years ago
Comment on attachment 651882 [details] [diff] [review]
Patch

Thanks Pike, I'll address your comment.
Attachment #651882 - Flags: review?(ttaubert)
(Assignee)

Comment 4

5 years ago
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.
Attachment #651882 - Attachment is obsolete: true
Attachment #664894 - Flags: feedback?(l10n)
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.
Attachment #664894 - Flags: feedback?(l10n) → feedback-
(Assignee)

Comment 6

5 years ago
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.
Attachment #664894 - Attachment is obsolete: true
Attachment #664928 - Flags: feedback?(l10n)
Comment on attachment 664928 [details] [diff] [review]
updated patch using iQ() API

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

Yeah, I like this better.
Attachment #664928 - Flags: feedback?(l10n) → feedback+
(Assignee)

Comment 8

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

Tim, could you review this patch please?
Thanks
Attachment #664928 - Flags: review?(ttaubert)
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.
Attachment #664928 - Flags: review?(ttaubert)
Attachment #664928 - Flags: review+
Attachment #664928 - Flags: feedback+
(Assignee)

Comment 10

5 years ago
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?
Assignee: nobody → pascalc
Attachment #664928 - Attachment is obsolete: true
Attachment #664952 - Flags: review+
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?
Attachment #664952 - Flags: review+ → review-
(Assignee)

Comment 12

5 years ago
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 :)
Attachment #664952 - Attachment is obsolete: true
Attachment #664973 - Flags: review?(ttaubert)
Comment on attachment 664973 [details] [diff] [review]
updated patch, for real now !

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

Nice, thanks :)
Attachment #664973 - Flags: review?(ttaubert) → review+
(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 :)
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/fx-team/rev/b56fa836e894
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 16

5 years ago
awesome, thanks Tim!
https://hg.mozilla.org/mozilla-central/rev/b56fa836e894
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
(Assignee)

Updated

5 years ago
Whiteboard: [mozfr]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.