Closed Bug 1532712 Opened 5 years ago Closed 5 years ago

Accesskeys of XBL elements with wrappable labels are no longer displayed

Categories

(Firefox :: Settings UI, defect, P1)

Product:
Firefox
Component:
Settings UI
Version:
67 Branch
Platform:
x86_64
Windows 10
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 67
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 + fixed

People

(Reporter: Gijs, Assigned: zbraniecki)

References

Details

(Keywords: nightly-community, regression)

Attachments

(3 files)

Bug 1532712 - Expose pause/resume observing methods on DocumentL10n. r?smaug
47 bytes, text/x-phabricator-request
Details | Review
Bug 1532712 - Optimize Preferences translation. r?gijs
47 bytes, text/x-phabricator-request
Details | Review
Bug 1532712 - Fix tests to await for gotoPref. r?gijs
47 bytes, text/x-phabricator-request
Details | Review

+++ This bug was initially created as a clone of Bug #1532071 +++

Steps To Reproduce:

  1. Open about:preferences
  2. scroll down to the "Files and applications" section

Expected result:
Underlined access keys for "Save files to" and "Always ask you where to save files" on Windows/Linux

Actual result:
No underlined access keys.

Based on https://searchfox.org/mozilla-central/search?q=%3C(xul%3A)%3Flabel&case=false&regexp=true&path=toolkit%2Fcontent%2Fwidgets%2F*.xml, these are the bindings with labels:

  • radio
  • various menu ones
  • button
  • toolbarbutton
  • tabbox
  • wizard

The only nodes in about:preferences with access keys based on this bit of JS:

[... new Set([... document.querySelectorAll("[accesskey]")].map(n => n.nodeName))]

are:

[ "checkbox", "label", "button", "radio", "treecol", "menuitem" ]

The treecol ones are the applications list, and they don't do anything, as far as I can tell, and even if they did they'd conflict with other ones. In fact, there's a lot of conflicts in en-US; it doesn't look like access keys were taken into account when we last reorg'd this stuff.

button and checkbox are working, as we already discussed. The only 2 menuitems with access keys are the ones in the "trackers" dropdown in the "custom" section of the content blocking prefs. They're working, as <menuitem> is using the value attribute of the label (see also bug 1532071 comment 4).

There are 4 labels and 9 radio elements with access keys, as far as I can see.

Gandalf, we're going to run into this more when we deal with fluent, because we're not set up to listen for the actual accesskey/label attributes not being present when the element is inserted in the DOM (e.g. I wonder how this will work with the current main window menubar patch, and if we're avoiding work there with fluent because we're not setting the access keys). Adding lots of mutation observers (one per item) seems very unfortunate. Sure, it's 13 items here but if we need this for all the menuitems in the main browser window we're going to do a lot of busy-work. Do you have better ideas? Was this problem considered in the design of fluent?

Flags: needinfo?(gandalf)

[Tracking Requested - why for this release]:
Basically the same regression as bug 1532071 so this needs to track 67 too.

tracking-firefox67: --- → ?

Is the problem about XBL not being ready for accesskey being inserted after parsing?

So, <foo accesskey="K"/> works but <foo> and foo.setAttribute("accesskey", "K") doesn't?

This will be a problem because we not only want to be able to inject translation asynchronously from Fluent initially, but also may want to change it in result of locale change.

Adding lots of mutation observers (one per item) seems very unfortunate.

Ugh, I'd definitely prefer not to do that. Is there no other way to set some inheritance in XBL?

Was this problem considered in the design of fluent?

I'm not sure if I understand the problem scope exactly, but I think we assumed that DOM is capable of being dynamically modified during the lifetime of the document and those changes are accurately reflected in the functionality. If that's not the case, we may have a problem :(

Flags: needinfo?(gandalf) → needinfo?(gijskruitbosch+bugs)

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

Is the problem about XBL not being ready for accesskey being inserted after parsing?

Yes. Well, in fact, there are 2 problems. That problem, and the fact that Fluent seems to run twice in about:preferences. Specifically, if I add logging in intl/l10n/DOMLocalization.jsm : https://searchfox.org/mozilla-central/rev/b2d35912da5b2acecb0274eb113777d344ffb75e/intl/l10n/DOMLocalization.jsm#187

that logs the ID, attr name and value, every node/attr/value combination is logged twice when about:preferences open.

This is obviously wasteful, but the problem is actually worse than that. In order to fix this bug, I need to somehow respond to the attributes being set, so I can set the accesskey property and make things work. The extant code that needs the find in page code to not run before l10n has does this by using the promise returned by translateElements.

So I spent an hour writing code there, and it didn't work. Then I checked in the debugger, and could clearly see my accesskeys appearing correctly. Once I hit "continue" in the debugger, they disappeared again.

It seems that the translateElements() translation happens first, then the promise gets resolved, and then fluent runs again for reasons I don't understand, and wipes out all the access keys again (note: access keys are set using <html:span> elements in the text contents of the inner <label> in the <radio>. The <label> xbl:inherits the label attribute value from the <radio>. So when fluent calls setAttribute("label", "..."), the XBL code wipes out the contents of the <label> element and replaces them with the string it's just been given.

So, <foo accesskey="K"/> works but <foo> and foo.setAttribute("accesskey", "K") doesn't?

Correct, when the accesskey gets set after the binding is initialized (which is the bit that bug 1520350 changed here) Note that <foo id="bar"> and document.getElementById("bar").accessKey = "K" also works, and is what other code uses.

This will be a problem because we not only want to be able to inject translation asynchronously from Fluent initially, but also may want to change it in result of locale change.

For which we need bug 1520659...

Adding lots of mutation observers (one per item) seems very unfortunate.

Ugh, I'd definitely prefer not to do that. Is there no other way to set some inheritance in XBL?

There is already inheritance happening, but the inheritance just sets the attribute. It doesn't call setters and it doesn't do any other work, whereas for accesskey underlining, such work needs to happen any time either the access key or the label changes. See also bug 325251.

Was this problem considered in the design of fluent?

I'm not sure if I understand the problem scope exactly, but I think we assumed that DOM is capable of being dynamically modified during the lifetime of the document and those changes are accurately reflected in the functionality. If that's not the case, we may have a problem :(

Right, this assumption does not hold in practice. We can make it so, of course (it's all just code), but that's more work.

Acutely, we need a solution for 67 which is about to branch. What can I do here? I thought that the hacky thing I detailed at the top was going to work, but it clearly doesn't and I don't see a good way of making it so short of needing bug 1520659 fixed on 67.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(gandalf)
See Also: → 1520659

So the Custom Element version of label (Bug 1448213, which I hope to land next merge) will fix the problem of accesskey attribute changes not being detected. It'll also detect textContent changes via XBL inheritance. It won't detect them from .textContent, but I have some ideas here we could do either on the CE side or on the Fluent side.

This will turn correctness problems into potential performance problems if there's a lot of attribute churn. One example is https://github.com/projectfluent/fluent.js/issues/300, where when an attribute isn't going to change ultimately, it ends up getting removed and then re-set to the prior value. This is bad for the label case since that means we format the access key, wipe it out, and then format it again. Once that's fixed, we should be able to do some profiling to confirm the new impl is performing well - Gijs, I'll loop you into that before landing the CE version.

That doesn't explain what to do for 67 though, maybe we can update fluent with a fix like landed in the CE base class in Bug 1532071 in the meantime? Anyway, I'll be online next week to talk more about it - maybe a synchronous chat would be helpful here.

Yes. Well, in fact, there are 2 problems. That problem, and the fact that Fluent seems to run twice in about:preferences. Specifically, if I add logging in intl/l10n/DOMLocalization.jsm : https://searchfox.org/mozilla-central/rev/b2d35912da5b2acecb0274eb113777d344ffb75e/intl/l10n/DOMLocalization.jsm#187
that logs the ID, attr name and value, every node/attr/value combination is logged twice when about:preferences open.

I think it's worth filing a bug about it, because it shouldn't.

It seems that the translateElements() translation happens first, then the promise gets resolved, and then fluent runs again for reasons I don't understand, and wipes out all the access keys again (note: access keys are set using <html:span> elements in the text contents of the inner <label> in the <radio>. The <label> xbl:inherits the label attribute value from the <radio>. So when fluent calls setAttribute("label", "..."), the XBL code wipes out the contents of the <label> element and replaces them with the string it's just been given.

That may be the same or separate bug, also worth filing (with STR please!)

Note that <foo id="bar"> and document.getElementById("bar").accessKey = "K" also works, and is what other code uses.

That's not what I see :(

As an experiment, I removed data-l10n-id from the element, so it looks like this:

      <radio id="saveTo"
             value="true"
             accesskey="v"
             label="Save files to"/>

This results in Save files to with an underline under v.

I then opened the console and typed document.getElementById("saveTo").setAttribute("label", "Save files to (2)"); - this results in accesskey underline being removed.

Which indicates to me that this bug is reproducible irrelevant of the double-translation bugs mentioned above, because just the action of setting the label from JS causes it.

Then I tried:

  • document.getElementById("saveTo").setAttribute("accesskey", "v");
  • document.getElementById("saveTo").accesskey = "v";

but they did not result in the underline reappearing.

If my understanding is correct, that means that Fluent cannot dynamically set label/accesskey of this element and get the underline working.

For which we need bug 1520659...

Possibly, depending if we'd try to make setAttributes return a Promise or Fluent to fire an event.

Acutely, we need a solution for 67 which is about to branch. What can I do here? I thought that the hacky thing I detailed at the top was going to work, but it clearly doesn't and I don't see a good way of making it so short of needing bug 1520659 fixed on 67.

Agree. I'm concerned that if the experiment I documented above doesn't work, there's nothing on the Fluent side of things I can do to make it so.
Am I missing something?

Flags: needinfo?(gandalf) → needinfo?(gijskruitbosch+bugs)

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #6)

Then I tried:

  • document.getElementById("saveTo").setAttribute("accesskey", "v");
  • document.getElementById("saveTo").accesskey = "v";

but they did not result in the underline reappearing.

I think ‘.accessKey = ...’ (with an uppercase K) worked last time I tried.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #6)

Yes. Well, in fact, there are 2 problems. That problem, and the fact that Fluent seems to run twice in about:preferences. Specifically, if I add logging in intl/l10n/DOMLocalization.jsm : https://searchfox.org/mozilla-central/rev/b2d35912da5b2acecb0274eb113777d344ffb75e/intl/l10n/DOMLocalization.jsm#187
that logs the ID, attr name and value, every node/attr/value combination is logged twice when about:preferences open.

I think it's worth filing a bug about it, because it shouldn't.

I think one is the "natural" result of appending content to the page that has data-l10n-ids on, and one is the result of calling translateElements. Is that unexpected?

Note that <foo id="bar"> and document.getElementById("bar").accessKey = "K" also works, and is what other code uses.

That's not what I see :(

As an experiment, I removed data-l10n-id from the element, so it looks like this:

      <radio id="saveTo"
             value="true"
             accesskey="v"
             label="Save files to"/>

This results in Save files to with an underline under v.

I then opened the console and typed document.getElementById("saveTo").setAttribute("label", "Save files to (2)"); - this results in accesskey underline being removed.

Yes, this is bug 325251, which I referened in my earlier comment.

Which indicates to me that this bug is reproducible irrelevant of the double-translation bugs mentioned above, because just the action of setting the label from JS causes it.

Depends on the order in which the attributes are set, and could be worked around by ensuring one re-calls node.accessKey = node.accessKey whenever we set the label of an element with an access key - but the problem is that the double-translation bugs are making it impossible to fix the issue, because I can't know when the attributes have all been set so I can then make the <label> do the right thing again...

Then I tried:

  • document.getElementById("saveTo").setAttribute("accesskey", "v");
  • document.getElementById("saveTo").accesskey = "v";

but they did not result in the underline reappearing.

As :ntim said, accessKey with a capital K.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(gandalf)

(In reply to :Gijs (he/him) from comment #8)

Then I tried:

  • document.getElementById("saveTo").setAttribute("accesskey", "v");
  • document.getElementById("saveTo").accesskey = "v";

but they did not result in the underline reappearing.

As :ntim said, accessKey with a capital K.

Oh, and it looks like you're calling it on the <radio>, it doesn't exist there, you need to call it on the anonymous <label> descendant.

Oh, and it looks like you're calling it on the <radio>, it doesn't exist there, you need to call it on the anonymous <label> descendant.

Ahh! Now I understand (I think!).

So, the problem is that we want to react to an accesskey being set on radio by looking into anonymous label inside it and setting it there. And we can't use inherit, so we need to somehow observe the accessKey attribute being set on radio and reset it onto the label whenever label or accessKey on radio is being set.

That's tricky... :/ I'm investigating.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #10)

Oh, and it looks like you're calling it on the <radio>, it doesn't exist there, you need to call it on the anonymous <label> descendant.

Ahh! Now I understand (I think!).

So, the problem is that we want to react to an accesskey being set on radio by looking into anonymous label inside it and setting it there. And we can't use inherit,

I don't really understand why you keep bringing up inheritance. The label and accesskey values are inherited to the anonymous label in the <radio> case (but that's not really relevant for the toplevel/non-anon <label>s that are also affected by this same bug, see the result of document.querySelectorAll("label[data-l10n-id][accesskey]:not(.checkbox-label)") in the console)... but that's only the attribute/textcontent.

The actual visible display of the access key is done using the formatAccessKey helper method, for labels that use their textContent to display an access key (labels that use their value attribute have the display of the underline etc. done directly in the XUL frame (C++) code, and aren't affected here). So either that helper needs calling or the accessKey property setter (which calls it) needs invoking. Neither is happening right now.

If/when we do call either of these to properly show the access key, and then somehow the label/accesskey is modified again, the accesskey underline is lost again, because it is implemented as an <html:span> node inside the <label>'s contents, which are overwritten when the label attribute on the <radio> is changed (or fluent changes the textcontent of a non-anonymous <label data-l10n-id="..."/>.

so we need to somehow observe the accessKey attribute being set on radio and reset it onto the label whenever label or accessKey on radio is being set.

The attribute is "accesskey", only the property is "accessKey".

That's tricky... :/ I'm investigating.

As I already said, it's fairly easy to just grab the set of modified elements and manually invoke these methods/setters, but there's no reliable way to know when fluent is done translating. Hence the link to bug 1520659. We're currently using manual translateElements and the promise that returns, but that seems to not stop us doing another translation pass with fluent, which runs into the issues I described previously.

My point is, from the <radio/> XBL perspective, it should not matter if the label/accesskey is set by Fluent, or by hand.

In theory, the encapsulated logic should fire the formatAccessKey whenever either of the attributes is updated.

You seem to argument that the solution is for us to monitor localization state, and fire the helper in result of the localization.

I'm arguing for separation of concerns. The common state handler between Fluent and XBL is the DOM of the <radio/> element. So IIUC whenever its attributes get updated (either by Fluent or by anything else), the internal logic should update the label with the accesskey.

If that works, then Fluent should work out of the box and the double-translation bug should not impact this experience.

If/when we do call either of these to properly show the access key, and then somehow the label/accesskey is modified again, the accesskey underline is lost again, because it is implemented as an <html:span> node inside the <label>'s contents, which are overwritten when the label attribute on the <radio> is changed (or fluent changes the textcontent of a non-anonymous <label data-l10n-id="..."/>.

Do we agree that it is a bug in the <radio/> internal implementation? Can we fix it on that level?

Flags: needinfo?(gandalf) → needinfo?(gijskruitbosch+bugs)

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #12)

My point is, from the <radio/> XBL perspective, it should not matter if the label/accesskey is set by Fluent, or by hand.

In theory, the encapsulated logic should fire the formatAccessKey whenever either of the attributes is updated.

You seem to argument that the solution is for us to monitor localization state, and fire the helper in result of the localization.

You yourself said you didn't want individual mutation observers (comment #3). There is no other way for the binding to know the attribute changed (that I'm aware of - and certainly no other way that wouldn't be on a per-element basis if we insist on "encapsulated logic" or "separation of concern").

Do we agree that it is a bug in the <radio/> internal implementation? Can we fix it on that level?

We do not, because it doesn't just affect <radio> elements...

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(gandalf)

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #12)

My point is, from the <radio/> XBL perspective, it should not matter if the label/accesskey is set by Fluent, or by hand.

This is fine from a theoretical purity perspective, but in practice we haven't run into this limitation pretty much at all, as far as I can tell, and nobody even bothered to actually land the fix for bug 325251. If individual JS wants to set the label/accesskey "by hand", it does so in order and using the .accessKey property (or before the XBL binding is constructed), and then things Just Work. Fluent is the odd one out because of how it generically changes things. I understand that you may not want to change fluent for what you may see as a "quirk" of XUL / the XBL binding, but that doesn't really mean that a "generic"/"encapsulated" solution is really best here.

You yourself said you didn't want individual mutation observers (comment #3). There is no other way for the binding to know the attribute changed (that I'm aware of - and certainly no other way that wouldn't be on a per-element basis if we insist on "encapsulated logic" or "separation of concern")

I see. Yes, I'd prefer not to have to set a MutationObserver for each element affected. I was hoping for some XBL setter inside the element's logic to react.

We do not, because it doesn't just affect <radio> elements...

Shouldn't all elements affected react in the same way?

Maybe let me rephrase my question. Would you agree that if we have label/accesskey pair on any element, then any case of setting them should update the label and its accesskey in associated label?

Or do you think we should not react to such DOM changes and design a special solution for when localization sets them?

I understand that you may not want to change fluent for what you may see as a "quirk" of XUL / the XBL binding, but that doesn't really mean that a "generic"/"encapsulated" solution is really best here.

I have no problem changing Fluent to handle XUL/XBL binding properly! I'm just trying to understand the scope of the problem first.

Would some sort of logic in Fluent like that work?

function localizeElement(elem, translation) {
  if (translation.attributes.hasOwnProperty("label") && translation.attributes.hasOwnProperty("accesskey") && elem.accessKey) {
    elem.setAttribute("label", translation.attributes["label"]);
    elem.accessKey = translation.attributes["accesskey"];
  }
}
Flags: needinfo?(gandalf) → needinfo?(gijskruitbosch+bugs)

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #15)

You yourself said you didn't want individual mutation observers (comment #3). There is no other way for the binding to know the attribute changed (that I'm aware of - and certainly no other way that wouldn't be on a per-element basis if we insist on "encapsulated logic" or "separation of concern")

I see. Yes, I'd prefer not to have to set a MutationObserver for each element affected. I was hoping for some XBL setter inside the element's logic to react.

Right. I'm not aware of any way to do that beyond a mutation observer or mutation events (which are just a worse form of mutation observers).

We do not, because it doesn't just affect <radio> elements...

Shouldn't all elements affected react in the same way?

Maybe let me rephrase my question. Would you agree that if we have label/accesskey pair on any element, then any case of setting them should update the label and its accesskey in associated label?

In theory, yes. In practice, we have a regression in 67 and we're 5 days from merge day, and overhauling a bunch of XUL/XBL infrastructure to do this seems risky. Note also that some (notably <checkbox>) but not all (notably <radio>) of these elements have been converted to custom elements, so they're not all affected in the same way.

Or do you think we should not react to such DOM changes and design a special solution for when localization sets them?

I think long-term having fluent and these DOM changes cooperate right would be preferred, though it's also a problem that's slowly going away as a result of the CE conversions. Short-term I think I might prefer fixing what we know is broken in a way that's safe.

The other problem here is that we keep running into cases where an event from fluent that indicates it just translated a bunch of stuff is useful, and we keep not adding it. Note that the prefs code already works around this in several places, so adding an event like that wouldn't just be for this case, although it'd help.

I understand that you may not want to change fluent for what you may see as a "quirk" of XUL / the XBL binding, but that doesn't really mean that a "generic"/"encapsulated" solution is really best here.

I have no problem changing Fluent to handle XUL/XBL binding properly! I'm just trying to understand the scope of the problem first.

Would some sort of logic in Fluent like that work?

function localizeElement(elem, translation) {
  if (translation.attributes.hasOwnProperty("label") && translation.attributes.hasOwnProperty("accesskey") && elem.accessKey) {
    elem.setAttribute("label", translation.attributes["label"]);
    elem.accessKey = translation.attributes["accesskey"];
  }
}

With some tweaks (notably: <radio> don't currently have an accessKey property, I think, and so we might have to add a forwarding prop; we may also need logic to avoid doing this for elements that have already been converted to custom elements), I think so.

(In reply to :Gijs (he/him) from comment #8)

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #6)

Yes. Well, in fact, there are 2 problems. That problem, and the fact that Fluent seems to run twice in about:preferences. Specifically, if I add logging in intl/l10n/DOMLocalization.jsm : https://searchfox.org/mozilla-central/rev/b2d35912da5b2acecb0274eb113777d344ffb75e/intl/l10n/DOMLocalization.jsm#187
that logs the ID, attr name and value, every node/attr/value combination is logged twice when about:preferences open.

I think it's worth filing a bug about it, because it shouldn't.

I think one is the "natural" result of appending content to the page that has data-l10n-ids on, and one is the result of calling translateElements. Is that unexpected?

Still wondering about this, too. :-)

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(gandalf)

The other problem here is that we keep running into cases where an event from fluent that indicates it just translated a bunch of stuff is useful, and we keep not adding it.

We don't add it because, frankly, I see no way of doing that in the pure JS world that is not prone to quite sophisticated mechanics. :( I'm hoping that as we move more code to native, we'll be able to start returning Promise from setAttributes.

With some tweaks (notably: <radio> don't currently have an accessKey property, I think, and so we might have to add a forwarding prop; we may also need logic to avoid doing this for elements that have already been converted to custom elements), I think so.

I'm happy to add such code. Am I correct that we need this only for label and radio as per your comment 1?

Still wondering about this, too. :-)

Yeah, still on my mind. I'll investigate it today.

I think one is the "natural" result of appending content to the page that has data-l10n-ids on, and one is the result of calling translateElements. Is that unexpected?

So, this seems to be as expected. My question is why are we calling translateElements on DOM that gets injected into the document observed by Fluent's MutationObserver?

If we have a good reason to do that, maybe we should:

await document.l10n.translateFragment(frag);
document.l10n.pauseObserving();
root.appendChild(frag);
document.l10n.resumeObserving();

?

Would that resolve the problem or do we need both - reduce the double-translation and set the accesskey either in result of some MozAfterTranslation event or via some special-case in translateElement?

Flags: needinfo?(gandalf) → needinfo?(gijskruitbosch+bugs)

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #18)

I think one is the "natural" result of appending content to the page that has data-l10n-ids on, and one is the result of calling translateElements. Is that unexpected?

So, this seems to be as expected. My question is why are we calling translateElements on DOM that gets injected into the document observed by Fluent's MutationObserver?

This is explained in this comment: https://searchfox.org/mozilla-central/rev/8ff2cd0a27e3764d9540abdc5a66b2fb1e4e9644/browser/components/preferences/in-content/preferences.js#52-63 .

It's a workaround for bug 1520659 not being fixed. Note that we do this all over the place by now:

https://searchfox.org/mozilla-central/search?q=translateElements&case=false&regexp=false&path=.js

because we need to ensure localization happens before e.g. dialog window sizing.

Is bug 1520659 likely to be fixed for 67?

If we have a good reason to do that, maybe we should:

await document.l10n.translateFragment(frag);
document.l10n.pauseObserving();
root.appendChild(frag);
document.l10n.resumeObserving();

We currently append before calling translateElements. I don't know what other side-effects there would be when we change that.

Would that resolve the problem or do we need both - reduce the double-translation and set the accesskey either in result of some MozAfterTranslation event or via some special-case in translateElement?

I think (but haven't tested) that would resolve the issue, because the XBL bindings shouldn't be constructed until we insert the content into the DOM, so then the access key will be correct from the beginning.

Of course that doesn't fix the locale change case, but I'm not super worried about that here.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(gandalf)

I was able to identify the culprit of why we localize twice, and reorganize the code to avoid double translation.

As a side effect, because we now inject the XUL already localized, we don't hit the label overriding accesskey accessor and that fixes the problem.

Flags: needinfo?(gandalf)

Zibi is driving this now, so taking this out of my list. Thanks!

Assignee: gijskruitbosch+bugs → gandalf

I'm having trouble to understand the proposed API. It feels very error prone. Appending elements to DOM has all sorts of side effects, which could cause more DOM mutations, but if we aren't observing mutations, those mutated parts won't get translated.
But I assume the API is just a temporary to get this bug fixed for 67.

I'm having trouble to understand the proposed API.

This is not a new API. I'm just exposing an API already existing on DOMLocalization and used internally:

It feels very error prone. Appending elements to DOM has all sorts of side effects, which could cause more DOM mutations, but if we aren't observing mutations, those mutated parts won't get translated.

That's exactly the purpose of the API. To suspend translations being captured by our MutationObserver when we know that they shouldn't be.

We used it in Firefox OS for exactly the same purpose - the Settings app wanted to lazily inject a DOM Fragment that we already localized prior to injection. So we wanted to pause observer, inject, resume observer.

But I assume the API is just a temporary to get this bug fixed for 67.

It is not.

Please, let me know if you have further concerns about this API. If you do, we may want to either rethink this approach or file a follow-up.

Flags: needinfo?(bugs)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5fb5272aec8e
Expose pause/resume observing methods on DocumentL10n. r=smaug
https://hg.mozilla.org/integration/autoland/rev/1af088f5fc34
Optimize Preferences translation. r=Gijs

As I said on IRC, the issue is semi-synchronous APIs called during the DOM mutation:
Mutation events and Custom Element Reactions and such.

Flags: needinfo?(bugs)

Backed out 2 changesets (bug 1532712) for mochitest failures at browser_policy_set_homepage.js on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/autoland/rev/b8c1398f5a9074d3741997199d3eb2fa568c5469

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&selectedJob=233661656&revision=1af088f5fc34935f780c2153f1d279ca482e735a

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=233661656&repo=autoland&lineNumber=2900

Log snippet:

[task 2019-03-13T17:45:59.327Z] 17:45:59 INFO - TEST-START | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js
[task 2019-03-13T17:46:00.761Z] 17:46:00 INFO - TEST-INFO | started process screentopng
[task 2019-03-13T17:46:01.233Z] 17:46:01 INFO - TEST-INFO | screentopng: exit 0
[task 2019-03-13T17:46:01.234Z] 17:46:01 INFO - Buffered messages logged at 17:45:59
[task 2019-03-13T17:46:01.235Z] 17:46:01 INFO - Entering test bound policies_headjs_startWithCleanSlate
[task 2019-03-13T17:46:01.235Z] 17:46:01 INFO - TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js | Engine is inactive at the start of the test -
[task 2019-03-13T17:46:01.237Z] 17:46:01 INFO - Leaving test bound policies_headjs_startWithCleanSlate
[task 2019-03-13T17:46:01.237Z] 17:46:01 INFO - Entering test bound homepage_test_simple
[task 2019-03-13T17:46:01.242Z] 17:46:01 INFO - TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js | Sanity check the temporary file doesn't exist. - true == true -
[task 2019-03-13T17:46:01.243Z] 17:46:01 INFO - TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js | Homepage URL should match expected -
[task 2019-03-13T17:46:01.248Z] 17:46:01 INFO - TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js | Lock status of browser.startup.homepage should match expected -
[task 2019-03-13T17:46:01.249Z] 17:46:01 INFO - Buffered messages finished
[task 2019-03-13T17:46:01.250Z] 17:46:01 INFO - TEST-UNEXPECTED-FAIL | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js | Uncaught exception - TypeError: homepageTextbox is null
[task 2019-03-13T17:46:01.251Z] 17:46:01 INFO - Leaving test bound homepage_test_simple
[task 2019-03-13T17:46:01.252Z] 17:46:01 INFO - Entering test bound homepage_test_repeat_same_policy_value
[task 2019-03-13T17:46:01.252Z] 17:46:01 INFO - TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js | Sanity check the temporary file doesn't exist. - true == true -
[task 2019-03-13T17:46:01.511Z] 17:46:01 INFO - TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js | Homepage URL should match expected -
[task 2019-03-13T17:46:01.512Z] 17:46:01 INFO - TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js | Lock status of browser.startup.homepage should match expected -
[task 2019-03-13T17:46:01.513Z] 17:46:01 INFO - TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js | Pref page value should match expected -
[task 2019-03-13T17:46:01.515Z] 17:46:01 INFO - TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js | Lock status of browser.startup.page should match expected -
[task 2019-03-13T17:46:02.080Z] 17:46:02 INFO - TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js | Disabled status of session restore status should match expected - false == false -
[task 2019-03-13T17:46:02.080Z] 17:46:02 INFO - TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js | Session restore status checkbox should be: checked - true == true -
[task 2019-03-13T17:46:02.096Z] 17:46:02 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-03-13T17:46:02.098Z] 17:46:02 INFO - TEST-UNEXPECTED-FAIL | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js | Uncaught exception - TypeError: homepageTextbox is null
[task 2019-03-13T17:46:02.099Z] 17:46:02 INFO - Leaving test bound homepage_test_repeat_same_policy_value
[task 2019-03-13T17:46:02.101Z] 17:46:02 INFO - Entering test bound homepage_test_different_policy_value
[task 2019-03-13T17:46:02.102Z] 17:46:02 INFO - TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js | Sanity check the temporary file doesn't exist. - true == true -
[task 2019-03-13T17:46:02.403Z] 17:46:02 INFO - TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js | Homepage URL should match expected -
[task 2019-03-13T17:46:02.404Z] 17:46:02 INFO - TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js | Lock status of browser.startup.homepage should match expected -
[task 2019-03-13T17:46:02.982Z] 17:46:02 INFO - Not taking screenshot here: see the one that was previously logged

Flags: needinfo?(gandalf)

Ugh, so all of those tests did gotoPref. I scanned searchfox and fortunately there doesn't seem to be many occurrences of that call and it was easy to switch them to async.

Full try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aef2d2a99abdc999b0c3fb494f0e7804dd8a106b

Flags: needinfo?(gandalf)

hmm, there seem to be one more test failure only reproducible in debug builds:

TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_decoderDoctor.js | A promise chain failed to handle a rejection: gHomePane is not defined - stack: init_all@chrome://browser/content/preferences/in-content/preferences.js:77:3

debugging further

I don't understand this bug.

In my last try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c97ed20c7312233b6de3903a805bbdcdcf1dc7bb&selectedJob=233755965

I see the fail in several forms:

TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_documentnavigation.js | A promise chain failed to handle a rejection: gMainPane is not defined - stack: init_all@chrome://browser/content/preferences/in-content/preferences.js:76:3

TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_decoderDoctor.js | A promise chain failed to handle a rejection: gContainersPane is not defined - stack: init_all@chrome://browser/content/preferences/in-content/preferences.js:80:3

TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_decoderDoctor.js | A promise chain failed to handle a rejection: gMainPane is not defined - stack: init_all@chrome://browser/content/preferences/in-content/preferences.js:76:3

Logically, that makes no sense:

  1. preferences.js is loaded synchronously first
  2. main.js is loaded after but still synchronously
  3. gMainPane is defined synchronously in main.js
  4. init_all is called in response to DOMContentLoaded
  5. somehow gMainPane is undefined at that point?

I have no idea how the async change may cause it :(

What's worse, I can't reproduce it locally in debug or opt build even with --verify. :(

I'm doing the last thing I can come up with: rebased on top of new central and refired the try hoping it'll disappear?
Just kidding. Hoping to see something more out of the log :/

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97cea3fcee86af8ee8a5cf9b0391f5e31e1d908a

If anyone has any idea how this error may be caused by those patches or leads on how to debug it, please help.

From a failing log:

[task 2019-03-13T22:57:27.816Z] 22:57:27 INFO - Console message: [JavaScript Error: "TypeError: this.windowElement is null" {file: "resource://gre/modules/DOMLocalization.jsm" line: 528}]
[task 2019-03-13T22:57:27.819Z] 22:57:27 INFO - Console message: [JavaScript Error: "ReferenceError: gMainPane is not defined" {file: "chrome://browser/content/preferences/in-content/preferences.js" line: 76}]

I expect that the document/window gets destroyed while we're midway through trying to do this stuff (e.g. because the tab gets closed quickly by a test, or at least before this code runs).

Thanks Gijs!

Smaug says that checking if ownerGlobal is null should help us here. Testing that - https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf747daac1a0a229030935a63a0ece64fe0dad37

I see the log from bailing in DOMLocalization now:

[task 2019-03-14T19:25:26.655Z] 19:25:26     INFO - GECKO(1070) | console.log: "Bailing out early from connectRoot!"

which removes the first warning, but unfortunately placing:

 async function init_all() {
+  if (ownerGlobal === null) {
+    console.log('Bailing out early!');
+    return;
+  }
+
   Preferences.forceEnableInstantApply();

doesn't seem to be enough to fix:

[task 2019-03-14T19:25:26.899Z] 19:25:26     INFO - GECKO(1070) | JavaScript error: chrome://browser/content/preferences/in-content/preferences.js, line 88: ReferenceError: gSyncPane is not defined

What confuses me is that those lines are called, synchronously, one after another:

  register_module("paneGeneral", gMainPane);
  register_module("paneHome", gHomePane);
  register_module("paneSearch", gSearchPane);
  register_module("panePrivacy", gPrivacyPane);
  register_module("paneContainers", gContainersPane);
  register_module("paneSync", gSyncPane);

If gMainPane is present, and gHomePane and so on, why it stops on gPrivacyPane only?

What confuses me is that I don't know if all of the g* are registered but somehow in between those calls the global gets destroyed, or we somehow stop registering the globals randomly at some point.

And... why does my patch at all affect it? Is it because it turns init_all to async and that gives the DOM a chance to start destroying global?

I'm testing this hypothesis by turning init_all into sync again - https://treeherder.mozilla.org/#/jobs?repo=try&revision=440df911945877bf803628b59fe6e086ac68b958

Ugh... ok, that's ugly.

So, my hypothesis from comment 36 is partially right. The change of init_all to async does cause the error to be reported.
With init_all sync the error just doesn't surface, but it's still there!

Here's my try: https://taskcluster-artifacts.net/W2OfKcfOTwqweDIbEhigpg/0/public/logs/live_backing.log

search for gSearchPane, but notice that bc13 is green in https://treeherder.mozilla.org/#/jobs?repo=try&revision=440df911945877bf803628b59fe6e086ac68b958&selectedJob=233963683

Now look at mozilla-central random try: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=858a58a6aa93e044e04de5ccdad4aae4b9840657

and the log from bc13: https://taskcluster-artifacts.net/C4meU3cHS8G9jkCvPE--8g/0/public/logs/live_backing.log

search for gSearchPane.

At this point I can either wrap init_all in try{}catch{} for 67 and report the bug to refactor register_module to be called from the modules rather than via the exposed globals after 67, or keep init_all sync and use the .then as I do in my patch.

I'll stick to the latter unless Gijs will prefer the former or something else.

The try looks green. Please, land at will once the final patch gets r+.

Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e076e178370
Expose pause/resume observing methods on DocumentL10n. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/10dfa9788b2b
Optimize Preferences translation. r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/adbb2fb5c8ec
Fix tests to await for gotoPref. r=gijs

https://hg.mozilla.org/mozilla-central/rev/9e076e178370
https://hg.mozilla.org/mozilla-central/rev/10dfa9788b2b
https://hg.mozilla.org/mozilla-central/rev/adbb2fb5c8ec

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox67: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: