Error when trying to restart into new language on Beta

RESOLVED FIXED in Firefox 64

Status

()

P1
normal
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: mstriemer, Assigned: mstriemer)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 65
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox64 fixed, firefox65 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
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
(Assignee)

Comment 1

4 months ago
Created attachment 9022645 [details]
Bug 1504725 - Explicitly pass event to language change handler r?jaws
(Assignee)

Comment 2

4 months ago
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
Flags: needinfo?(bugs)

Comment 3

4 months ago
Pushed by mstriemer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84367a375806
Explicitly pass event to language change handler r=jaws
(Assignee)

Comment 4

4 months ago
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
Attachment #9022645 - Flags: approval-mozilla-beta?
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.
Flags: needinfo?(bugs)
(Assignee)

Comment 6

4 months ago
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?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)

Comment 8

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/84367a375806
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
bug 1505031 is another case of beta-only breakage due to this footgun.  That's pretty scary...
Comment on attachment 9022645 [details]
Bug 1504725 - Explicitly pass event to language change handler r?jaws

trivial fix, approved for 64.0b8
Attachment #9022645 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Updated

3 months ago
See Also: → bug 1504890
(Assignee)

Comment 12

3 months ago
There's a lint rule from bug 1504890 now, so hopefully it doesn't happen again.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.