1457252 - Application details sub-dialog does not display buttons properly on Italian locale

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Stefan [:StefanG_QA]]

Attachment: Applications section sub-dialog does not display buttons properly.JPG

[Affected versions]: 
Nightly 60.0a1

[Affected platforms]:
Platforms: Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.

[Steps to reproduce]:
1. Go to "about:preferences" using Italian locale
2. Under "Application" press the irc (action section drop-down) => Always ask => Application Details
3. Observe the dialog

AR: Application details sub-dialog does not display the buttons properly
ER: Application details sub-dialog should be displayed properly

Comment 1

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

Attachment: Screenshot Firefox 60b15 Windows 10 it

Can't reproduce it on Windows 10. Flod, can you?

Comment 2

[Stefan [:StefanG_QA]]

Attachment: Screen Shot 2018-04-26 at 2.04.33 PM.png

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:61.0) Gecko/20100101 Firefox/61.0 20180426100055

I was able to reproduce similar issue in OS X 10.13 with the following sub-dialogs:

1. Open Nightly (EN) with New Profile 
2. About:preferencec#privacy 
3. Under Tracking Protection press Change Block list... button
4. Observe 
--------------------------------
1. Open Nightly (EN) with New Profile 
2. About:preferencec#privacy 
3. Under Cookies and Site Data press Clear Data... button
4. Observe 
--------------------------------
1. Open Nightly (French) with New Profile 
2. About:preferencec#home 
3. Set Homepage and new window to Custom URLs...
4. Press Use Bookmark... button
4. Observe 
--------------------------------
In either case the buttons are not visible or truncated. However once I close and re-open the sub-dialog the issue is gone. I can NOT reproduce it again with the same profile. I need to create a new one.

Comment 3

[Stefan [:StefanG_QA]]

Attachment: Screen Shot 2018-04-26 at 2.07.44 PM.png

Comment 4

[Stefan [:StefanG_QA]]

Attachment: Screen Shot 2018-04-26 at 2.08.29 PM.png

Comment 5

[Francesco Lodolo [:flod]]

(In reply to Stefan [:StefanG_QA] from comment #0)
> [Affected versions]: 
> Nightly 60.0a1

I assume that's Nightly 61.

I tried reproducing comment 2 on macOS, but I can't. Tried multiple new profiles, even reducing the window size, but dialog are always correctly sized.

I can reproduce comment 0 though, but it's unrelated to Fluent. I can reproduce it on 60.0b15 (64 bit), which doesn't have subdialogs migrated, and I could reproduce it while testing those patches (I thought I filed a bug, but apparently I didn't).

Comment 6

[Mike Conley (:mconley) (:⚙️)]

I seem to recall bug 1439875 causing something like this elsewhere, but I don't exactly remember where... I wonder if that's related.

A regression window would be super great here. StefanG_QA, since you're able to reproduce this, are you able to use mozregression to get us a regression range?

Comment 7

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

Taking. I can reproduce and I have a patch

Comment 8

[Stefan [:StefanG_QA]]

I've used mozregression to narrow down the regression range. Here are my findings:

No more inbound revisions, bisection finished.
Last good revision: 7a26b1da04e517e9abad958d8add815e4b47e378
First bad revision: 27e293c662c840e6aa2bcd81d3ce4f301293f425
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7a26b1da04e517e9abad958d8add815e4b47e378&tochange=27e293c662c840e6aa2bcd81d3ce4f301293f425

Comment 9

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

Attachment: Bug 1457252 - Ensure l10n and initialization are done before sizing the subdialog.

Review commit: https://reviewboard.mozilla.org/r/240856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240856/

Comment 10

[Code Review Bot [:reviewbot]]

Comment on attachment 8972192 [details]
Bug 1457252 - Ensure l10n and initialization are done before sizing the subdialog.

https://reviewboard.mozilla.org/r/240856/#review246640


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/preferences/applicationManager.js:10
(Diff revision 1)
>  /* import-globals-from in-content/main.js */
>  
>  var gAppManagerDialog = {
>    _removed: [],
>  
> -  init: function appManager_init() {
> +  onLoad: function() {

Error: Expected method shorthand. [eslint: object-shorthand]

Comment 11

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

Ok, so I cannot reproduce it, but I can confirm that the current launch of subdialogs is racy and Fluent will make it more racy in the future  (bug 1457948 for example).

The reason is that the subdialogs.js synchronously uses onLoad event to trigger measuring of the dialog.
That means that all of the content in the dialog that affects sizing must be in DOM by the time load event is fired.

That may not happen in two cases:

1) Any async operation in the subdialog is performed before content is injected into the DOM
2) Localization has to trigger fallback locale which asynchronously triggers I/O.

I toyed with an event, but ended up switching the subdialogs._onLoad to be async and optionally blocked on two promises:

a) document.l10n.ready is a regular Promise that gets resolved when initial document localization is completed.
b) a custom document.mozSubdialogReady Promise can be added to further delay initialization.

The (a) is imediatelly affecting all subdialogs since they use Fluent and it removes a potential race condition (2).
(b) is more of an easy way to further extend this delay if needed. Currently I only use it in one subdialog, but in bug 1457948 I'll want it for more.

Comment 12

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

Having a simple patch makes me want to suggest it for 61. It's not a huge regression and hopefully won't be visible if we don't get it into 61, but the cases in which the regression may be visible are kind of edgy like minority (incomplete) locales, so it'd be nice to have a general fix in place.

Comment 13

[:Gijs (out for now; he/him)]

Comment on attachment 8972192 [details]
Bug 1457252 - Ensure l10n and initialization are done before sizing the subdialog.

https://reviewboard.mozilla.org/r/240856/#review246666

I'm not sure I understand why we need **2** promises, both of which involving l10n...

::: browser/components/preferences/applicationManager.js:10
(Diff revision 1)
> -  init: function appManager_init() {
> +  onLoad: function() {
> +    document.mozSubdialogReady = this.init();
> +  },
> +
> +  init: async function appManager_init() {

Please just make both of these use method shorthand.

::: browser/components/preferences/applicationManager.js:35
(Diff revision 1)
>        });
>      }
>  
> +    // We want to block on this element being localized because the
> +    // result will impact the size of the subdialog.
> +    await document.l10n.translateFragment(appDescElem);

If we already wait for l10n.ready, why is this necessary?

Comment 14

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

> I'm not sure I understand why we need **2** promises, both of which involving l10n...

I tried to capture that in the docstring, but clearly didn't do a good job. Let me try here and may need you help with the docstring :)

1) document.l10n.ready is a Promise available on every FluentDOM localized document that is resolved once the initial translations is completed. In most cases this happens *way* before DOMContentLoaded and "load" event, but since its async it *can* in extreme cases happen later.

The first guard makes sure that we only display the subdialog after the document is localized.

2) document.mozSubdialogReady is a Promise that optionally can be added to make subdialogs.js wait for it.
This Promise allows any subdialog to add *custom* async initialization states.

Now, this custom state *can* be localization. For example in onLoad we can do:

```
document.l10n.setAttribute(mainDesc, "subdialog-description", {
  unreadEmails: 5
});
await document.l10n.translateFragment(mainDesc);
```

which happens after the document is localized so cannot be guarded by (1).

but there's in this patch even, an example of a different async operation uses at the startup:

```
await SiteDataManager.getTotalUsage().then(bytes => {
  let [amount, unit] = DownloadUtils.convertByteUnits(bytes);
  document.l10n.setAttributes(this._clearSiteDataCheckbox,
    "clear-site-data-cookies-with-data", { amount, unit });
  });
await SiteDataManager.getCacheSize().then(bytes => {
  let [amount, unit] = DownloadUtils.convertByteUnits(bytes);
  document.l10n.setAttributes(this._clearCacheCheckbox,
    "clear-site-data-cache-with-data", { amount, unit });
  });
await document.l10n.translateElements([
  this._clearCacheCheckbox,
  this._clearSiteDataCheckbox
]);
```

Here, the readon those two elements are not translated from DOM and are not covered by document.l10n.ready, but instead are localized in `init` is because we need to perform an asynchronous operation to retrieve the data usage and only then insert the localization.

There are other cases in subdialogs where the initialization code wants to add their own async initialization code.

I could replace the latter `mozSubdialogReady` with:

```
onLoad() {
  document.l10n.ready = document.l10n.ready.then(this.init());
}
```

but it feels to me like it would conflate two separate states - the `this.init` asynchronousity may but doesn't have to be related to l10n. And `document.l10n.ready` is only communicating when the DOM has been localized.
Since it's a live system there can be further DOM changes triggering further localizations, but the `document.l10n.ready` is about the initial.

On top of that, if the system was performance critical, like it is in case of Preferences main window, I'd be reluctant to block UI on a promise, because it's ok to be racy - if some error causes our I/O to be slow, we can show Preferences before l10n is ready - it's ok to have suboptimal UX in an error scenario.

But here, if I get the system correctly, we have two differences:

1) subdialogs sizing is quirky. Most of DOM can be filled with content and will adapt. Subdialogs get one sizing and set the iframe's height and that's it. We need to get the final DOM before that.
2) subdialogs are not perf-critical. If we display them 60ms later, it's ok.

Is that ok with you?

Comment 15

[:Gijs (out for now; he/him)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #14)
> > I'm not sure I understand why we need **2** promises, both of which involving l10n...
> 
> I tried to capture that in the docstring, but clearly didn't do a good job.
> Let me try here and may need you help with the docstring :)
> 
> 1) document.l10n.ready is a Promise available on every FluentDOM localized
> document that is resolved once the initial translations is completed. In
> most cases this happens *way* before DOMContentLoaded and "load" event, but
> since its async it *can* in extreme cases happen later.
> 
> The first guard makes sure that we only display the subdialog after the
> document is localized.
> 
> 2) document.mozSubdialogReady is a Promise that optionally can be added to
> make subdialogs.js wait for it.
> This Promise allows any subdialog to add *custom* async initialization
> states.
> 
> Now, this custom state *can* be localization. For example in onLoad we can
> do:
> 
> ```
> document.l10n.setAttribute(mainDesc, "subdialog-description", {
>   unreadEmails: 5
> });
> await document.l10n.translateFragment(mainDesc);
> ```
> 
> which happens after the document is localized so cannot be guarded by (1).

Can we change it so the additional localization happens before and becomes part of (1) ?

I buy that we need 2 promises here, but it's confusing that in practice, both await l10n stuff.

That said, another relatively simple way to fix this would also be to just wait for promise (2) (never waiting for l10n.ready unless we really need to in order to get the sizing right, in which case (2) can just await (1) ).


> But here, if I get the system correctly, we have two differences:
> 
> 1) subdialogs sizing is quirky. Most of DOM can be filled with content and
> will adapt. Subdialogs get one sizing and set the iframe's height and that's
> it. We need to get the final DOM before that.
> 2) subdialogs are not perf-critical. If we display them 60ms later, it's ok.
> 
> Is that ok with you?

Well, I think the other fundamental problem here is that we never re-evaluate this afterwards. Sometimes XUL sizing, esp. with multiline text, is basically "not right" the first time, and then rewraps text and then the height we got the first time is "off". While waiting longer to determine the height fixes this as long as we wait long enough, in a sense perhaps it would be even better if we detected this after the fact and adjusted the iframe sizing at that point. Does an overflow event fire here, I wonder? Perhaps that could be used to check if the document height had changed since it was last sampled, and if so, update iframe sizing.

Comment 16

[:Gijs (out for now; he/him)]

(In reply to :Gijs (he/him) from comment #15)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #14)
> > But here, if I get the system correctly, we have two differences:
> > 
> > 1) subdialogs sizing is quirky. Most of DOM can be filled with content and
> > will adapt. Subdialogs get one sizing and set the iframe's height and that's
> > it. We need to get the final DOM before that.
> > 2) subdialogs are not perf-critical. If we display them 60ms later, it's ok.
> > 
> > Is that ok with you?
> 
> Well, I think the other fundamental problem here is that we never
> re-evaluate this afterwards.

That is to say, I'm not sure that we should just accept (1) in this list as a fact of life. We control all the variables here. :-)

Comment 17

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

> Can we change it so the additional localization happens before and becomes part of (1) ?

Not really.

We optimize the DOM localization to happen *very* early - it currently gets triggered in `MozBeforeInitialXULLayout`.

You'd have to make the subdialog's JS alter the DOM before that event happens. With Mossop's work in bug 1455649 we will perform the initial localization not from within of the document, but from nsIDocument.

That means that trying to inject DOM changes before the initial DOM localization happens is going to be very hard.

I think it's the right choice. Delaying DOM localization to allow for startup code to alter it means delaying it for everyone to make a few cases easier.

> That said, another relatively simple way to fix this would also be to just wait for promise (2) (never waiting for l10n.ready unless we really need to in order to get the sizing right, in which case (2) can just await (1) ).

That would make (2) mandatory, right? Because otherwise, if you don't wait for (1) and (2) is not present you're back to the race condition where fallback localization may cause incorrect iframe sizing.

> While waiting longer to determine the height fixes this as long as we wait long enough, in a sense perhaps it would be even better if we detected this after the fact and adjusted the iframe sizing at that point. 

That would be ideal, I agree. But I would prefer not to block fixing a regression on this since the resulting refactor will likely be larger and will need more testing and we're closing the cycle.

Maybe we could land this in some form and then file a follow up to work on the dynamic resizing in 62?

Comment 18

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

Comment on attachment 8972192 [details]
Bug 1457252 - Ensure l10n and initialization are done before sizing the subdialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240856/diff/1-2/

Comment 19

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

Comment on attachment 8972192 [details]
Bug 1457252 - Ensure l10n and initialization are done before sizing the subdialog.

https://reviewboard.mozilla.org/r/240856/#review246666

> If we already wait for l10n.ready, why is this necessary?

Because this element gets `data-l10n-id` assigned way later than DOM localization happens.

Localization happens in most cases (remember, it's async for a reason) in result of MozBeforeInitialXULLayout. This is called in onLoad way later.

We can let the next AnimationFrame localize it, but since we care about sizing, we trigger the translation manually to ensure `this.init` finishes (and subdialogs do sizing) after the element is localized.

Instead of this we could do `await new Promise(resolve => window.requestAnimationFrame(resolve));` if you prefer, but I find it less clear.

Comment 20

[:Gijs (out for now; he/him)]

Comment on attachment 8972192 [details]
Bug 1457252 - Ensure l10n and initialization are done before sizing the subdialog.

https://reviewboard.mozilla.org/r/240856/#review246960

r=me because it fixes the issue at hand, though it's a bit sad that the side-effect is likely to be slower dialog appearance. Gandalf, can you file a follow-up so we are able to resize the dialogs if their size changes after they load, which will allow us to unblock showing/sizing them for the initial load a bit more again?

::: browser/components/preferences/clearSiteData.js:25
(Diff revision 2)
> -    SiteDataManager.getTotalUsage().then(bytes => {
> +    // We'll block init() on this because the result values may impact
> +    // subdialog sizing.
> +    await SiteDataManager.getTotalUsage().then(bytes => {
>        let [amount, unit] = DownloadUtils.convertByteUnits(bytes);
>        document.l10n.setAttributes(this._clearSiteDataCheckbox,
>          "clear-site-data-cookies-with-data", { amount, unit });
>      });
> -    SiteDataManager.getCacheSize().then(bytes => {
> +    await SiteDataManager.getCacheSize().then(bytes => {
>        let [amount, unit] = DownloadUtils.convertByteUnits(bytes);
>        document.l10n.setAttributes(this._clearCacheCheckbox,
>          "clear-site-data-cache-with-data", { amount, unit });
>      });

Can you use Promise.all to make these bits execute simultaneously, rather than running them sequentially?

Comment 21

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

Comment on attachment 8972192 [details]
Bug 1457252 - Ensure l10n and initialization are done before sizing the subdialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240856/diff/2-3/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b70479effe0d
Ensure l10n and initialization are done before sizing the subdialog. r=Gijs

Comment 23

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

Filed bug 1458733.

Tested the patch with missing translations in applicationManager and clearSiteData subdialogs. Both handled it very well.

Comment 24

[Dorel Luca [:dluca]]

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

Comment 25

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm this issue is fixed, I verified using Fx 61.0b11, on Windows 7 x64.