Closed Bug 1472716 Opened 6 years ago Closed 6 years ago

Convert the listbox in "languages.xul" to "richlistbox"

Categories

(Firefox :: Settings UI, enhancement, P1)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(2 files)

Bug 1472716 - Part 2 - Convert the listbox in "languages.xul" to "richlistbox".
59 bytes, text/x-review-board-request
bgrins
: review+
zbraniecki
: review+
jaws
: review+
Details
Bug 1472716 - Part 1 - Whitelist the "value" attribute for XUL localization.
59 bytes, text/x-review-board-request
zbraniecki
: review+
tomvangoethem
: review+
Details 
This is a slightly more complex case of bug 1472554.
For the Fluent modifications, what approach should I use here, where the same string is used for different attributes in different places?

Even if this was the correct approach, it isn't working as expected for me, the "value" attribute isn't set on the inner label. Is this a bug, or is there something missing?

I'm asking Brian for the rest of the code review since Jared is away.
Flags: needinfo?(gandalf)
Note that I reproduced bug 1194346 by removing the workaround before the patch, and it now appears to be fixed by virtue of using "richlistbox" instead of "listbox".
Depends on: 1194346
(In reply to :Paolo Amadini from comment #2)
> For the Fluent modifications, what approach should I use here, where the
> same string is used for different attributes in different places?

I'm not sure if I understand and unfortunately the patch doesn't explain it. Are you trying to do:

```
<element value="Localizable Value"/>
<other-element label="Localizable Value"/>
```

or:

```
<element value="Localizable Value" label="Localizable Value"/>
```

?

Also, in almost any case, storing a localizable message as a program value seems like a bad idea - those should be used only for displaying.

> Even if this was the correct approach, it isn't working as expected for me,
> the "value" attribute isn't set on the inner label. Is this a bug, or is
> there something missing?

What is an "inner label"?
Flags: needinfo?(gandalf) → needinfo?(paolo.mozmail)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #4)
> <element value="Localizable Value"/>
> <other-element label="Localizable Value"/>

This, where the values are the same and are formatted with options, in this case "{ $locale } [{ $code }]".

> > Even if this was the correct approach, it isn't working as expected for me,
> > the "value" attribute isn't set on the inner label. Is this a bug, or is
> > there something missing?
> 
> What is an "inner label"?

It's the "var label = document.createElement("label");", on which I call this:

  document.l10n.setAttributes(label, "languages-code-format-value", { ... });

Note that the line before setAttributes in the patch is just for testing so you see something on the screen, the items would just be empty otherwise.
Flags: needinfo?(paolo.mozmail) → needinfo?(gandalf)
> This, where the values are the same and are formatted with options, in this case "{ $locale } [{ $code }]".

Is the `value` used anywhere in the code, or is it the label's `value` for display only?

In Fluent we use the concept of a `shape` to describe the structure of the localizable arguments for an UI element.

In your case, if I understand you have two shapes: `{value: null, args: ["label"]}` and `{value: null, args: ["value"]}` - those should use different localization messages, but it is ok for them to reference each other:

```
message-shape-one =
    .label = { $locale } [{ $code }]

message-shape-two =
    .value = { message-shape-one.label }
```

Fluent will resolve the variables in the context of arguments passed to each one of them including nesting :)

Does it solve your problem?
Flags: needinfo?(gandalf)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #6)
> Is the `value` used anywhere in the code, or is it the label's `value` for
> display only?

They're used only for display, using the "value" and "label" attributes.

> Fluent will resolve the variables in the context of arguments passed to each
> one of them including nesting :)
> 
> Does it solve your problem?

It tells me that having a different message referencing the original is the accepted solution, so it solves half of the problem. The other half is that this doesn't seem to work in my patch, the value of the label in the "richlistitem" is not set. I guess translateFragment was supposed to set the final "value" attribute? Did you notice similar issues while migrating Preferences to Fluent?
Flags: needinfo?(gandalf)
> The other half is that this doesn't seem to work in my patch, the value of the label in the "richlistitem" is not set. I guess translateFragment was supposed to set the final "value" attribute?

Can you try setting up `data-l10n-attrs="value"`? We have a pretty strict whitelist of allowed attributes [0] and if you need to use an attribute that is not whitelisted, you need to whitelist it separately.
This is for security reasons.

[0] https://searchfox.org/mozilla-central/source/intl/l10n/DOMLocalization.jsm#60
Flags: needinfo?(gandalf)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #8)
> Can you try setting up `data-l10n-attrs="value"`?

That was it. I'm surprised that this hasn't come up before in the Preferences conversion, but it seems that the labels in the main interface are mostly "description" elements whose innerText is used instead of the "value" attribute.

Brian, maybe we should start supporting <label label=""> and <description label=""> as a fallback for <label value=""> and <description value="">. This would simplify the conversion to Fluent, since many strings already use a shape with the ".value" attribute.

This would have simplified things here since we might have kept just one localizable string, as it was originally implemented. I expect more of these situations in the future.
Flags: needinfo?(bgrinstead)
(In reply to :Paolo Amadini from comment #9)
> a shape with the ".value" attribute.

..a shape with the ".label" attribute.
Paolo: another thing you can do is ass `"label": ["value"]` to the whitelist for XUL. I bet we can get it approved by security.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #12)
> Paolo: another thing you can do is ass `"label": ["value"]` to the whitelist
> for XUL. I bet we can get it approved by security.

Can you clarify what this will do exactly?
Jared, this review is probably for you when you're back :-)
Flags: needinfo?(jaws)
(In reply to :Paolo Amadini from comment #9)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #8)
> > Can you try setting up `data-l10n-attrs="value"`?
> 
> That was it. I'm surprised that this hasn't come up before in the
> Preferences conversion, but it seems that the labels in the main interface
> are mostly "description" elements whose innerText is used instead of the
> "value" attribute.
> 
> Brian, maybe we should start supporting <label label=""> and <description
> label=""> as a fallback for <label value=""> and <description value="">.
> This would simplify the conversion to Fluent, since many strings already use
> a shape with the ".value" attribute.
> 
> This would have simplified things here since we might have kept just one
> localizable string, as it was originally implemented. I expect more of these
> situations in the future.

I'd prefer to not add an alias for [value] in order to keep the API simpler. If we had to change the attribute name, I'd rather do a full find/replace in the tree from .value and [value] to .label and [label]. As you mention, this is the type of thing that may come up more in the future, so good to think through what we'll want to do as a rule.

Zibi: would your suggestion in Comment 12 allow us to proceed without changing the <label> and <description> element's API to support [label] in addition to [value]?
Flags: needinfo?(bgrinstead) → needinfo?(gandalf)
> Zibi: would your suggestion in Comment 12 allow us to proceed without changing the <label> and <description> element's API to support [label] in addition to [value]?

Yes. My proposal is to add `label: ["value"]` to [0] which will make Fluent accept value attribute as localizable on every `label` element in XUL without having to whitelist it per case with `data-l10n-attrs`.


[0] https://searchfox.org/mozilla-central/source/intl/l10n/DOMLocalization.jsm#66
Flags: needinfo?(gandalf)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #17)
> > Zibi: would your suggestion in Comment 12 allow us to proceed without changing the <label> and <description> element's API to support [label] in addition to [value]?
> 
> Yes. My proposal is to add `label: ["value"]` to [0] which will make Fluent
> accept value attribute as localizable on every `label` element in XUL
> without having to whitelist it per case with `data-l10n-attrs`.
> 
> 
> [0]
> https://searchfox.org/mozilla-central/source/intl/l10n/DOMLocalization.jsm#66

OK, that seems reasonable. I'd say let's try for that, and with `description` elements as well.
Okay, I'll post updated patches for review. Arguably, this use of the "value" attribute for presentation text is an exception, and we may want to do a mass migration back to "label" when we move our implementation from XUL to HTML with Custom Elements, but I think that's far in the future, and we can always use migration scripts for the localized files if we'll need it.
Comment on attachment 8989223 [details]
Bug 1472716 - Part 2 - Convert the listbox in "languages.xul" to "richlistbox".

https://reviewboard.mozilla.org/r/254268/#review262856

:jaws - feel free to have a look as well, but this version looks good to me

::: browser/components/preferences/languages.js
(Diff revision 4)
>    writeSpoofEnglish() {
>      return document.getElementById("spoofEnglish").checked ? 2 : 1;
>    }
>  };
> -
> -// These focus and resize handlers hack around XUL bug 1194844

Nice! Let's make sure to close out https://bugzilla.mozilla.org/show_bug.cgi?id=1194844 when this gets resolved
Attachment #8989223 - Flags: review?(bgrinstead) → review+
Comment on attachment 8991003 [details]
Bug 1472716 - Part 1 - Whitelist the "value" attribute for XUL localization.

https://reviewboard.mozilla.org/r/256012/#review262950

Sweet, r+ from me, but I'd like to get a security team rubber stamp as well.
Attachment #8991003 - Flags: review?(gandalf) → review+
Comment on attachment 8991003 [details]
Bug 1472716 - Part 1 - Whitelist the "value" attribute for XUL localization.

Tom, pauljt pointed at you as stepping in for FreddyB while he's on PTO. Freddy did the whole Fluent review and this is just a small extension to whitelist label.value and description.value as localizable attributes.

It should be a rather no-brainer and a safe extension unless there are known cases where those two attributes are used for things different than localization text.
Attachment #8991003 - Flags: review?(tomvangoethem)
Comment on attachment 8989223 [details]
Bug 1472716 - Part 2 - Convert the listbox in "languages.xul" to "richlistbox".

https://reviewboard.mozilla.org/r/254268/#review262976
Attachment #8989223 - Flags: review+
Flags: needinfo?(jaws)
Comment on attachment 8989223 [details]
Bug 1472716 - Part 2 - Convert the listbox in "languages.xul" to "richlistbox".

https://reviewboard.mozilla.org/r/254268/#review263004

lgtm!
Attachment #8989223 - Flags: review?(gandalf) → review+
Comment on attachment 8991003 [details]
Bug 1472716 - Part 1 - Whitelist the "value" attribute for XUL localization.

https://reviewboard.mozilla.org/r/256012/#review263382

LGTM. I don't see any security-sentive issues raised by this.
Attachment #8991003 - Flags: review?(tomvangoethem) → review+
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c33114c93dd0
Part 1 - Whitelist the "value" attribute for XUL localization. r=gandalf
https://hg.mozilla.org/integration/mozilla-inbound/rev/340fb9189eed
Part 2 - Convert the listbox in "languages.xul" to "richlistbox". r=jaws,gandalf
https://hg.mozilla.org/mozilla-central/rev/c33114c93dd0
https://hg.mozilla.org/mozilla-central/rev/340fb9189eed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Blocks: 1491646
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: