Closed
Bug 782779
Opened 12 years ago
Closed 12 years ago
Using simple quotes for GroupItem's placeholder breaks some localized strings
Categories
(Firefox Graveyard :: Panorama, defect)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 18
People
(Reporter: theo, Assigned: pascalc)
References
Details
(Whiteboard: [mozfr])
Attachments
(1 file, 4 obsolete files)
1.58 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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)
Updated•12 years ago
|
Assignee: theo.chevalier11 → nobody
Status: ASSIGNED → NEW
Comment 2•12 years ago
|
||
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•12 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•12 years ago
|
||
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 5•12 years ago
|
||
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•12 years ago
|
||
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 7•12 years ago
|
||
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•12 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 9•12 years ago
|
||
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•12 years ago
|
||
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 11•12 years ago
|
||
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•12 years ago
|
||
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 13•12 years ago
|
||
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+
Comment 14•12 years ago
|
||
(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
Comment 15•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b56fa836e894
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 16•12 years ago
|
||
awesome, thanks Tim!
Comment 17•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b56fa836e894
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
Assignee | ||
Updated•12 years ago
|
Whiteboard: [mozfr]
Updated•8 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•