1495357 - migrate wizard-buttons binding to CE

Status: RESOLVED FIXED

(Core :: XUL, task, P3)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: wizard dtd to fluent

does it look about right?

Comment 2

[Francesco Lodolo [:flod]]

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 >"

Comment 3

[alexander :surkov (:asurkov)]

Attachment: wizard dtd to fluent

Comment 4

[alexander :surkov (:asurkov)]

Attachment: wizard binding to CE

Comment 5

[alexander :surkov (:asurkov)]

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?

Comment 6

[Francesco Lodolo [:flod]]

Sorry, need to redirect, that's beyond my technical understanding of Fluent.

Comment 7

[Zibi Braniecki [:zbraniecki][: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.

Comment 8

[alexander :surkov (:asurkov)]

(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?

Comment 9

[alexander :surkov (:asurkov)]

Attachment: 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)

Comment 10

[Brian Grinstead [:bgrins]]

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;
    }

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

> 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 :)

Comment 12

[Brian Grinstead [:bgrins]]

(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.

Comment 13

[alexander :surkov (:asurkov)]

(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.

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

> 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

[:Paolo Amadini]

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

Adding Brian in case he gets to this review before me...

Comment 16

[:Paolo Amadini]

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.

Comment 17

[alexander :surkov (:asurkov)]

Attachment: Bug 1495357 - convert wizard-buttons binding to a Custom Element, r=bgrins

Comment 18

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov (:asurkov) from comment #17)

Created attachment 9050787 [details]
Bug 1495357 - convert wizard-buttons binding to a Custom Element, r=bgrins

It has mochitest failures https://treeherder.mozilla.org/#/jobs?repo=try&revision=c30f43bded218d37bd3e22f27f1c74b4a721c5d0&selectedJob=234870145, I cannot reproduce locally, for example this one:

TEST-UNEXPECTED-ERROR | testing/firefox-ui/tests/puppeteer/test_update_wizard.py TestUpdateWizard.test_elements | NoSuchElementException: Unable to locate element: [dlgtype="cancel"].

Could it be because buttons Custom Element is not yet attached, when button is accessed?

Comment 19

[Brian Grinstead [:bgrins]]

Does the test pass locally, or are you not able to run it locally? I have had to do some marionette updates in the past when changing XBL anon content to normal DOM (for example https://hg.mozilla.org/mozilla-central/rev/04b57ffa53ce#l26.1).

Comment 20

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #18)

(In reply to alexander :surkov (:asurkov) from comment #17)

Created attachment 9050787 [details]
Bug 1495357 - convert wizard-buttons binding to a Custom Element, r=bgrins

It has mochitest failures https://treeherder.mozilla.org/#/jobs?repo=try&revision=c30f43bded218d37bd3e22f27f1c74b4a721c5d0&selectedJob=234870145, I cannot reproduce locally, for example this one:

TEST-UNEXPECTED-ERROR | testing/firefox-ui/tests/puppeteer/test_update_wizard.py TestUpdateWizard.test_elements | NoSuchElementException: Unable to locate element: [dlgtype="cancel"].

Could it be because buttons Custom Element is not yet attached, when button is accessed?

Possibly, if the element hasn't become visible and XBL hasn't yet constructed (since we rely on the CE upgrade call inside XBL constructor).

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33510bccced0
convert wizard-buttons binding to a Custom Element, r=bgrins

Comment 22

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/33510bccced0