1504725 - Error when trying to restart into new language on Beta

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Mark Striemer [:mstriemer]]

Nothing happens when clicking the "Apply and restart" button after switching browser languages on Beta.

There is an error in the console: ReferenceError: event is not defined

Comment 1

[Mark Striemer [:mstriemer]]

Attachment: Bug 1504725 - Explicitly pass event to language change handler r?jaws

Comment 2

[Mark Striemer [:mstriemer]]

Smaug, do you know why this worked on Nightly but doesn't work on Beta? It seems concerning that the `event` object is available in Nightly but not on Beta.

I tried to do a mozregression with jaws's help it doesn't appear to work with mozilla-beta right now.

The relevant bits seem to be:

  <!-- in-content/main.xul [1] -->
  <button class="message-bar-button" oncommand="gMainPane.confirmBrowserLanguageChange()"/>

  // in-content/main.js [2]
  var gMainPane = { // ...
    confirmBrowserLanguageChange() {
      // `event` is undefined here.
      let localesString = (event.target.getAttribute("locales") || "").trim();
      // ...
    },
  };

[1] https://searchfox.org/mozilla-central/rev/7c848ac7630df5baf1314b0c03e015683599efb9/browser/components/preferences/in-content/main.xul#303
[2] https://searchfox.org/mozilla-central/rev/7c848ac7630df5baf1314b0c03e015683599efb9/browser/components/preferences/in-content/main.js#806

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84367a375806
Explicitly pass event to language change handler r=jaws

Comment 4

[Mark Striemer [:mstriemer]]

Comment on attachment 9022645 [details]
Bug 1504725 - Explicitly pass event to language change handler r?jaws

No QE needed since this is behind a pref and there are other bugs that will be verified (currently waiting to test those blocked on this one).

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: The restart into new language feature won't work.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Makes implicit argument passed explicitly.

String changes made/needed: None

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Yeah, window.event is old IEism and shouldn't be used. We're adding support for it just for web compatibility. Note, window.event isn't available for example when dealing with Shadow DOM.

Comment 6

[Mark Striemer [:mstriemer]]

Just got the mozregression working, tracked it down to bug 1493869. So having this work on Nightly but not Beta is expected but sounds like this bug could easily happen again.

Is there something we should do to catch these bugs before they hit Beta? Should we add a lint rule for this?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

If the issue could be caught by lint, then yes. It is in general bad practice to rely on
"global variable" -like window.event. And as I said, window.event won't work when dealing with shadow DOM.

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/84367a375806

Comment 9

[Julien Cristau [:jcristau]]

bug 1505031 is another case of beta-only breakage due to this footgun.  That's pretty scary...

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 9022645 [details]
Bug 1504725 - Explicitly pass event to language change handler r?jaws

trivial fix, approved for 64.0b8

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2b9ec060ca1a

Comment 12

[Mark Striemer [:mstriemer]]

There's a lint rule from bug 1504890 now, so hopefully it doesn't happen again.