migrate wizard-buttons binding to CE

NEW
Assigned to

Status

()

P3
normal
4 months ago
3 months ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

4 months ago
Created attachment 9013235 [details] [diff] [review]
wizard dtd to fluent

does it look about right?
Assignee: nobody → surkov.alexander
Attachment #9013235 - Flags: review?(francesco.lodolo)
Comment on attachment 9013235 [details] [diff] [review]
wizard dtd to fluent

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

I would need to look at the code to see if you're picking the right attributes, but it seems to be going in the right direction.

But please test your migration, and your FTL file for syntax issues
https://projectfluent.org/play/

::: python/l10n/fluent_migrations/bug_1495357
@@ +1,1 @@
> +# coding=utf8

Filename needs to include .py, and some info about the bug, e.g. "bug_1495357_wizard.py"

@@ +8,5 @@
> +from fluent.migrate.helpers import transforms_from
> +
> +
> +def migrate(ctx):
> +    """Bug 1495357 - migrate wizard-buttons binding to CE."""

https://firefox-source-docs.mozilla.org/intl/l10n/l10n/fluent_migrations.html#basic-migration

You're missing the `, part {index}.` here. Also, no clue what "CE" is?

@@ +17,5 @@
> +        transforms_from(
> +"""
> +button-back-mac =
> +    .label = { COPY(from_path, "button-back-mac.label") }
> +button-back-mac =

This doesn't work, it should simply be like the FTL file

```
button-back-mac =
    .label = { COPY(from_path, "button-back-mac.label") }
    .accesskey = { COPY(from_path, "button-back-mac.accesskey") }
```

::: toolkit/locales/en-US/toolkit/main-window/wizard.ftl
@@ +17,5 @@
> +
> +button-cancel-mac =
> +  .label = Cancel
> +
> +button-back-unix

Missing = sign

@@ +21,5 @@
> +button-back-unix
> +  .label = Back
> +  .accesskey = B
> +
> +button-next-unix

Missing = sign

@@ +25,5 @@
> +button-next-unix
> +  .label = Next
> +  .accesskey = N
> +
> +button-finish-unix = 

Get rid of the unnecessary trailing whitespaces through the file.

@@ +32,5 @@
> +button-cancel-unix =
> +  .label = Cancel
> +
> +button-back-win = 
> +  .label = < Back

You should use the character, not HTML entities here

```
button-back-win = 
   .label = < Back
```

@@ +36,5 @@
> +  .label = &lt; Back
> +  .accesskey = B
> +
> +button-next-win = 
> +  .label = Next &gt;

Same here: "Next >"
Attachment #9013235 - Flags: review?(francesco.lodolo) → feedback+

Updated

4 months ago
Priority: -- → P3
(Assignee)

Comment 3

4 months ago
Created attachment 9013492 [details]
wizard dtd to fluent
Attachment #9013235 - Attachment is obsolete: true
(Assignee)

Comment 4

4 months ago
Created attachment 9013493 [details] [diff] [review]
wizard binding to CE
(Assignee)

Comment 5

4 months ago
Francesco, does fluent work from XBL anonymous content? I assume it doesn't, at least the patch doesn't seem working :) Anyways, wizard binding will be converted to Custom Elements, which makes wizard-buttons under a shadow DOM. Does Fluent work over shadow DOM?
Flags: needinfo?(francesco.lodolo)
Sorry, need to redirect, that's beyond my technical understanding of Fluent.
Flags: needinfo?(francesco.lodolo) → needinfo?(gandalf)
> Francesco, does fluent work from XBL anonymous content?

It can, but you need to add some JS logic.

> Does Fluent work over shadow DOM?

Yes, you just need to either handle it manually [0], or by adding the root of the shadowDOM to the document's DOMLocalization instance via `document.l10n.connectRoot` in the constructor.
Flags: needinfo?(gandalf)
(Assignee)

Comment 8

4 months ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #7)
> > Francesco, does fluent work from XBL anonymous content?
> 
> It can, but you need to add some JS logic.

got it, probably not worth it though

> > Does Fluent work over shadow DOM?
> 
> Yes, you just need to either handle it manually [0], or by adding the root
> of the shadowDOM to the document's DOMLocalization instance via
> `document.l10n.connectRoot` in the constructor.

I do call it from CE constructor and get an error:

TypeError: document.l10n is null; can't access its "connectRoot" property

any ideas what it could be?
Flags: needinfo?(gandalf)
(Assignee)

Comment 9

3 months ago
Created attachment 9017754 [details] [diff] [review]
DTD based wizard buttons CE

couldn't make it working with fluent, so sticking with DTD for now (I will file a follow up for fluent part)
Flags: needinfo?(gandalf)
Attachment #9017754 - Flags: review?(paolo.mozmail)
Comment on attachment 9017754 [details] [diff] [review]
DTD based wizard buttons CE

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

::: toolkit/content/widgets/wizard.js
@@ +36,5 @@
> +
> +const kDTDs = [ "chrome://global/locale/wizard.dtd" ];
> +
> +class MozWizardButtons extends MozXULElement {
> +  connectedCallback() {

Unless if there's a particular reason not to here, please use:

    if (this.delayConnectedCallback()) {
      return;
    }

as in https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/toolkit/content/widgets/progressmeter.js#72-74 to avoid issues with scripts running during parse (i.e. Bug 1495946).

@@ +155,5 @@
> +      [ "hidefinishbutton", "lastpage" ] : [];
> +  }
> +
> +  attributeChangedCallback(name, oldValue, newValue) {
> +    if (AppConstants.platform == "macosx") {

And here:

    if (!this.isConnectedAndReady) {
      return;
    }
> couldn't make it working with fluent, so sticking with DTD for now (I will file a follow up for fluent part)

I looked at the patch trying to understand the logic there.

You likely don't see `document.l10n` because you didn't add `<link rel="localization" href="..."/>` to that document so Fluent is not enabled.

But there's a deeper issue that we're discussing for a while now. Since CE don't ship l10n resources on their own, and there's no easy way to do that, we'd prefer to supply l10n from the document that embeds CE.

This means, that the "API" of the CE should expose the way to provide l10n resources, for example like this:

```
<window>
  <linkset>
    <link rel="localization" href="..."/>
  </linkset>

  <xul:wizard-buttons class="wizard-buttons" anonid="Buttons" xbl:inherits="pagestep,firstpage,lastpage">
    <button id="cancel" data-l10n-id="wizard-buttons-cancel"/>
    <button id="back" data-l10n-id="wizard-buttons-back"/>
    <button id="next" data-l10n-id="wizard-buttons-next"/>
    <button id="finish" data-l10n-id="wizard-buttons-finish"/>
  </xul:wizard-buttons>
```

This way all the l10n happens in the document, and CE just inherits the l10n attributes from those 4 buttons into its internal DOM.

Fluent gives you platform independent strings (so, no more -unix vs -mac etc.), and live locale switching etc.

Let me know if that helps :)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #11)
> > couldn't make it working with fluent, so sticking with DTD for now (I will file a follow up for fluent part)
>
> You likely don't see `document.l10n` because you didn't add `<link
> rel="localization" href="..."/>` to that document so Fluent is not enabled.
> 
> But there's a deeper issue that we're discussing for a while now. Since CE
> don't ship l10n resources on their own, and there's no easy way to do that,
> we'd prefer to supply l10n from the document that embeds CE.
> 
> This means, that the "API" of the CE should expose the way to provide l10n
> resources, for example like this:

That's what MozXULElement.insertFTLIfNeeded should be doing. It can safely be called multiple times (i.e. in a constructor: https://searchfox.org/mozilla-central/source/toolkit/content/widgets/findbar.js#17) and it will inject the localization link into the document the first time it's called.
(Assignee)

Comment 13

3 months ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #11)

> But there's a deeper issue that we're discussing for a while now. Since CE
> don't ship l10n resources on their own, and there's no easy way to do that,
> we'd prefer to supply l10n from the document that embeds CE.

mm, it is sort of inconvenient to require a developer to include two files (js and flt) instead one (js). I sort of like an idea when js takes care of it itself, and if no referred l10n file was found, then js just fails.

> This means, that the "API" of the CE should expose the way to provide l10n
> resources, for example like this:
> 
> ```
> <window>
>   <linkset>
>     <link rel="localization" href="..."/>
>   </linkset>
> 
>   <xul:wizard-buttons class="wizard-buttons" anonid="Buttons"
> xbl:inherits="pagestep,firstpage,lastpage">
>     <button id="cancel" data-l10n-id="wizard-buttons-cancel"/>
>     <button id="back" data-l10n-id="wizard-buttons-back"/>
>     <button id="next" data-l10n-id="wizard-buttons-next"/>
>     <button id="finish" data-l10n-id="wizard-buttons-finish"/>
>   </xul:wizard-buttons>

curious, whether CE connnectedCallback may trigger before linkset is processed, then it would make no l10n available.

(In reply to Brian Grinstead [:bgrins] from comment #12)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #11)
> > > couldn't make it working with fluent, so sticking with DTD for now (I will file a follow up for fluent part)
> >
> > You likely don't see `document.l10n` because you didn't add `<link
> > rel="localization" href="..."/>` to that document so Fluent is not enabled.
> > 
> > But there's a deeper issue that we're discussing for a while now. Since CE
> > don't ship l10n resources on their own, and there's no easy way to do that,
> > we'd prefer to supply l10n from the document that embeds CE.
> > 
> > This means, that the "API" of the CE should expose the way to provide l10n
> > resources, for example like this:
> 
> That's what MozXULElement.insertFTLIfNeeded should be doing. It can safely
> be called multiple times (i.e. in a constructor:
> https://searchfox.org/mozilla-central/source/toolkit/content/widgets/findbar.
> js#17) and it will inject the localization link into the document the first
> time it's called.

I'm curious whether <linkset> is processed async, it would explain why there's no l10n after insertFTLIfNeeded call.
> mm, it is sort of inconvenient to require a developer to include two files (js and flt) instead one (js).

Yeah, I totally understand that that's how it feels, but if you think about it more... it wouldn't be one js and one ftl, it would be one js and 100 ftl files (one per locale).

That doesn't scale well :(

> I sort of like an idea when js takes care of it itself, and if no referred l10n file was found, then js just fails.

Yeah, I like it as well. the trick it how to put 100 locales into the component and keep them up to date. I hope that in the future we'll have some sort of data model for Web Components that will enable that, but for now it's pretty tricky to do that :/

> curious, whether CE connnectedCallback may trigger before linkset is processed, then it would make no l10n available.

Only if the `<linkset/>` was placed after the component in the XUL/HTML code. We parse the `<linkset/>` synchronously.

> I'm curious whether <linkset> is processed async, it would explain why there's no l10n after insertFTLIfNeeded call.

No, it's not. Is your `document.l10n` still null after you injected the linkset? What if you place it manually in the XUL?

Comment 15

3 months ago
Comment on attachment 9017754 [details] [diff] [review]
DTD based wizard buttons CE

Adding Brian in case he gets to this review before me...
Attachment #9017754 - Flags: review?(bgrinstead)

Comment 16

3 months ago
Comment on attachment 9017754 [details] [diff] [review]
DTD based wizard buttons CE

Sorry for the long delay with the review flags here, as I understand it there is still ongoing discussion about how to port this to Fluent, and the buttons patch is still based on the wizard one, which is not ready for review yet, although they could be reordered.

After looking at the code, it seems to me that we might want to move the buttons to Fluent either before or during the conversion to Custom Element. Fluent handles platform differences in strings without the need for different markup, and similarly to what I did with download.xml in bug 1452629, simplifying how this works before the conversion may result in reduced complexity.

If this isn't viable and we end up needing substantially different structure and strings, we may be able to handle that with subclassing or by defining entirely different classes based on platform, rather than using "if" statements in each method.
Attachment #9017754 - Flags: review?(paolo.mozmail)
Attachment #9017754 - Flags: review?(bgrinstead)
You need to log in before you can comment on or make changes to this bug.