Francesco Lodolo [:flod] Reporter
	Description • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/b907280a1894#l5.18

# LOCALIZATION NOTE (trickle_caption_msg, trickle_highlight_color_name):
# ICE candidates arriving after the remote answer arrives are considered
# trickled (an attribute of an ICE candidate).  These are highlighted in
# the ICE stats table with light blue background.  This message is split
# into two in order to highlight the word "blue" with a light blue
# background to visually match the trickled ICE candidates.
trickle_caption_msg = trickled candidates (arriving after answer) are highlighted in
trickle_highlight_color_name = blue

This is not properly localizable, because it assumes a grammar structure for the sentence. Other languages need the flexibility to move around that color name.

Note that you need a new ID for the string, since this already landed in m-c. 
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

# LOCALIZATION NOTE (trickle_caption_msg2, trickle_highlight_color_name2): ICE
# candidates arriving after the remote answer arrives are considered trickled
# (an attribute of an ICE candidate). These are highlighted in the ICE stats
# table with light blue background. %S is replaced by
# trickle_highlight_color_name2 ("blue"), highlighted with a light blue
# background to visually match the trickled ICE candidates.
trickle_caption_msg2 = trickled candidates (arriving after answer) are highlighted in %S
trickle_highlight_color_name2 = blue

Normally I would ask for a backout, but given how many parts are in that patch, it doesn't seem fair.

	Francesco Lodolo [:flod] Reporter
	Comment 1 • 7 years ago

Forgot one question: is there a reason for the message starting lowercase?

	Michael Froman [:mjf] Assignee
	Comment 2 • 7 years ago

(In reply to Francesco Lodolo [:flod] from comment #0)
> https://hg.mozilla.org/mozilla-central/rev/b907280a1894#l5.18
> 
> # LOCALIZATION NOTE (trickle_caption_msg, trickle_highlight_color_name):
> # ICE candidates arriving after the remote answer arrives are considered
> # trickled (an attribute of an ICE candidate).  These are highlighted in
> # the ICE stats table with light blue background.  This message is split
> # into two in order to highlight the word "blue" with a light blue
> # background to visually match the trickled ICE candidates.
> trickle_caption_msg = trickled candidates (arriving after answer) are
> highlighted in
> trickle_highlight_color_name = blue
> 
> This is not properly localizable, because it assumes a grammar structure for
> the sentence. Other languages need the flexibility to move around that color
> name.
> 
> Note that you need a new ID for the string, since this already landed in
> m-c. 
> https://developer.mozilla.org/en-US/docs/Mozilla/Localization/
> Localization_content_best_practices#Changing_existing_strings
> 
> # LOCALIZATION NOTE (trickle_caption_msg2, trickle_highlight_color_name2):
> ICE
> # candidates arriving after the remote answer arrives are considered trickled
> # (an attribute of an ICE candidate). These are highlighted in the ICE stats
> # table with light blue background. %S is replaced by
> # trickle_highlight_color_name2 ("blue"), highlighted with a light blue
> # background to visually match the trickled ICE candidates.
> trickle_caption_msg2 = trickled candidates (arriving after answer) are
> highlighted in %S
> trickle_highlight_color_name2 = blue
> 
> Normally I would ask for a backout, but given how many parts are in that
> patch, it doesn't seem fair.

The issue with your proposed solution is that it doesn't allow me to highlight the word blue with the same background highlight as the trickled candidates.  That is the original reason I didn't use the %S in the string.  Would the following be acceptable?
trickle_caption_msg2 = trickled candidates (arriving after answer) are highlight color:
trickle_highlight_color_name2 = blue

	Michael Froman [:mjf] Assignee
	Comment 3 • 7 years ago

(In reply to Francesco Lodolo [:flod] from comment #1)
> Forgot one question: is there a reason for the message starting lowercase?
No.  That is a typo on my part.  I'll fix it when I fix this bug.

	Francesco Lodolo [:flod] Reporter
	Comment 4 • 7 years ago

(In reply to Michael Froman [:mjf] from comment #2)
> The issue with your proposed solution is that it doesn't allow me to
> highlight the word blue with the same background highlight as the trickled
> candidates.

Uhm, I confess I'm not familiar with that code. 

Would you be able to assign attributes to an existing <span> in the string, instead of creating it?

trickle_caption_msg2 = Trickled candidates (arriving after answer) are highlighted in <span>blue</span>

Or replace %S with the full <span> code?

	Michael Froman [:mjf] Assignee
	Comment 5 • 7 years ago

(In reply to Francesco Lodolo [:flod] from comment #4)
> (In reply to Michael Froman [:mjf] from comment #2)
> > The issue with your proposed solution is that it doesn't allow me to
> > highlight the word blue with the same background highlight as the trickled
> > candidates.
> 
> Uhm, I confess I'm not familiar with that code. 
> 
> Would you be able to assign attributes to an existing <span> in the string,
> instead of creating it?
> 
> trickle_caption_msg2 = Trickled candidates (arriving after answer) are
> highlighted in <span>blue</span>
> 
> Or replace %S with the full <span> code?

Neither of those 2 options are viable as far as I've been able to tell by experimenting with live code.  Is my proposal in comment 2 unacceptable (barring different wording choices)?

	Francesco Lodolo [:flod] Reporter
	Comment 6 • 7 years ago

trickle_caption_msg2 = Trickled candidates (arriving after answer) are highlighted with color:
trickle_highlight_color_name2 = blue

While this would work, it also feels really forced, to the point that I'd prefer having a natural sounding sentence and dropping the highlighting function (but that's a product decision).

I'm surprised that there are no ways out of this on the code side – e.g. setting .innerHTML instead of .textContent, as ugly as that would be – but I'm also out of my depth when it comes to that code.

	Francesco Lodolo [:flod] Reporter
	Comment 7 • 7 years ago

(In reply to Francesco Lodolo [:flod] from comment #6)
> While this would work, it also feels really forced, to the point that I'd
> prefer having a natural sounding sentence and dropping the highlighting
> function (but that's a product decision).

Clarification, since this sentence came out quite confusing: I'm referring to highlighting the word blue in the description.

	Michael Froman [:mjf] Assignee
	Comment 8 • 7 years ago

(In reply to Francesco Lodolo [:flod] from comment #6)
> I'm surprised that there are no ways out of this on the code side – e.g.
> setting .innerHTML instead of .textContent, as ugly as that would be – but
> I'm also out of my depth when it comes to that code.
Let me try .innerHTML - I didn't see that originally because it isn't on HTMLElement, but on the parent class, Element.

	:Gijs (he/him)
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8938451 [details]
Bug 1426130 - make trickle_caption_msg localizable.

https://reviewboard.mozilla.org/r/209158/#review214954

Code lgtm here, see note below on the old strings.

::: toolkit/locales/en-US/chrome/global/aboutWebrtc.properties:107
(Diff revision 1)
>  # ICE candidates arriving after the remote answer arrives are considered
>  # trickled (an attribute of an ICE candidate).  These are highlighted in
>  # the ICE stats table with light blue background.  This message is split
>  # into two in order to highlight the word "blue" with a light blue
>  # background to visually match the trickled ICE candidates.
>  trickle_caption_msg = trickled candidates (arriving after answer) are highlighted in

I kinda assume you need to remove the previous incarnation of this message? Flod will know for sure.

	Francesco Lodolo [:flod] Reporter
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8938451 [details]
Bug 1426130 - make trickle_caption_msg localizable.

https://reviewboard.mozilla.org/r/209158/#review214938

::: toolkit/content/aboutwebrtc/aboutWebrtc.js:645
(Diff revision 1)
> +    // it around the token, and builds the spans for each portion of the
> +    // caption.  This is to allow localization to put the color name for
> +    // the highlight wherever it is appropriate in the translated string
> +    // while avoiding innerHTML warnings from eslint.
> +    let captionTemplate = getString("trickle_caption_msg2");
> +    let [start, end] = captionTemplate.split(/%(?:1\$)?S/i);

Placeables can only be %S or %1$S, case sensitive. 

%s will actually give you fun truncations…

::: toolkit/content/aboutwebrtc/aboutWebrtc.js:650
(Diff revision 1)
> +    let [start, end] = captionTemplate.split(/%(?:1\$)?S/i);
> +
> +    // only append span if non-whitespace chars present
> +    if (/\S/.test(start)) {
> -    caption.appendChild(
> +      caption.appendChild(
> -        renderElement("span", `${getString("trickle_caption_msg")} `));
> +          renderElement("span", `${start} `));

I assume this adds a trailing space to the first part of the string. Do you actually need it? You're splitting the string at the placeable (%S), so you already have a trailing space.

::: toolkit/content/aboutwebrtc/aboutWebrtc.js:659
(Diff revision 1)
>            className: "trickled"
>          }));
> +    // only append span if non-whitespace chars present
> +    if (/\S/.test(end)) {
> +      caption.appendChild(
> +          renderElement("span", `${end} `));

Unnecessary trailing space?

::: toolkit/locales/en-US/chrome/global/aboutWebrtc.properties:113
(Diff revision 1)
>  # trickled (an attribute of an ICE candidate).  These are highlighted in
>  # the ICE stats table with light blue background.  This message is split
>  # into two in order to highlight the word "blue" with a light blue
>  # background to visually match the trickled ICE candidates.
>  trickle_caption_msg = trickled candidates (arriving after answer) are highlighted in
>  trickle_highlight_color_name = blue

You need to remove these two strings and the associated comment. They become unused at this point.

::: toolkit/locales/en-US/chrome/global/aboutWebrtc.properties:121
(Diff revision 1)
> +# candidates arriving after the remote answer arrives are considered trickled
> +# (an attribute of an ICE candidate). These are highlighted in the ICE stats
> +# table with light blue background. %1$S is replaced by
> +# trickle_highlight_color_name2 ("blue"), highlighted with a light blue
> +# background to visually match the trickled ICE candidates.
> +trickle_caption_msg2 = Trickled candidates (arriving after answer) are highlighted in %1$S

nit: use %S instead of %1$S, since there's only one placeable (and update the comment accordingly)

	Michael Froman [:mjf] Assignee
	Comment 12 • 7 years ago
mozreview-review-reply

Comment on attachment 8938451 [details]
Bug 1426130 - make trickle_caption_msg localizable.

https://reviewboard.mozilla.org/r/209158/#review214938

> I assume this adds a trailing space to the first part of the string. Do you actually need it? You're splitting the string at the placeable (%S), so you already have a trailing space.

Took those strait from Gijs' suggestion, but you're right.  I removed the extra space.

	Francesco Lodolo [:flod] Reporter
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8938451 [details]
Bug 1426130 - make trickle_caption_msg localizable.

https://reviewboard.mozilla.org/r/209158/#review214980

Looks good to me, thanks.

	Pulsebot
	Comment 16 • 7 years ago

Pushed by mfroman@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/f52f7f3fcaec
make trickle_caption_msg localizable. r=flod,Gijs

	:Gijs (he/him)
	Comment 17 • 7 years ago

(In reply to Michael Froman [:mjf] from comment #12)
> Comment on attachment 8938451 [details]
> Bug 1426130 - make trickle_caption_msg localizable.
> 
> https://reviewboard.mozilla.org/r/209158/#review214938
> 
> > I assume this adds a trailing space to the first part of the string. Do you actually need it? You're splitting the string at the placeable (%S), so you already have a trailing space.
> 
> Took those strait from Gijs' suggestion, but you're right.  I removed the
> extra space.

Ah, yeah, my bad! Sorry for the confusion...

	Andreea Pavel [:apavel]
	Comment 18 • 7 years ago
bugherder

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