Closed Bug 1454323 Opened 6 years ago Closed 6 years ago

Cmd+Q and other shortcuts are not working in some locales from Preferences

Categories

(Firefox :: Settings UI, defect, P1)

Unspecified
macOS
defect

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 + fixed
firefox61 --- fixed

People

(Reporter: flod, Assigned: zbraniecki)

Details

(Keywords: qawanted, regression, regressionwindow-wanted)

Attachments

(1 file)

Realized while testing Japanese Nightly: if I open Preferences, focus one of the tabs and try to quit, I can't. The focus moves to the search field, and nothing happens.

The same if I press other keyboard combinations, like cmd+T or cmd+N.

At first I thought about an issue in the locale, but it happens also on Hebrew.
- Nightly: ar, he, ja affected. Italian is NOT affected, which makes things more confusing.
- Release for Japanese (59.0.2) is fine, so it's a regression
60 beta has the same issue. Marking as macOS only, since I have no way to test other platforms at the moment.
OS: Unspecified → Mac OS X
It gets weirder: I can reproduce easily with Nightly Japanese. 

But, if I open the Console before Preferences, then it starts working. And I see a bunch of warnings related to missing strings. 

Can it be caused by the log functions? It would also explain why Italian works (no missing strings).

@zibi
I have the feeling this is related to our work.
Flags: needinfo?(gandalf)
Some more details.

Scenario #1:
- Open Nightly
- Open Preferences
- Try to open a new tab with cmd+T

Result: it doesn't work, search field is focused.

If I open the console at this point, and close the main window, I get a weird "Navigated to chrome://extensions/content/dummy.xul". Also, the application menu is completely gone, there is only "Firefox Nightly".
If I close the Console, menu comes back.

If I open a new window, open the console, then Preferences, everything works as expected.

Scenario #2.
- Open Nightly
- Open Console with cmd+shift+J
- Open Preferences
- Try to open a new tab with cmd+T

Result: everything works as normal.
On second thoughts: 60 only has the General pane migrated to Fluent. All the rest of work, e.g. bug 1445084 on Search Results or bug 1420761, was done in 61, so it doesn't explain the issue showing up on Beta.

French Nightly is affected too.
Summary: Cmd+Q and other shortcuts are not working in some locales → Cmd+Q and other shortcuts are not working in some locales from Preferences
This seems pretty bad. Tentatively marking this P1 for now.
Priority: -- → P1
flod, are you able to get a smaller regression window?
Flags: needinfo?(francesco.lodolo)
The first DevEdition (60b1) already has the problem, same for the fist official beta (60b3).

I tried to pick a bunch of Nightlies, but it takes forever to download and extract from here. I don't think I can get smaller than this
https://ftp.mozilla.org/pub/firefox/nightly/2018/02/
https://ftp.mozilla.org/pub/firefox/nightly/2018/03/

Working
60.0a1 (2018-02-27) (64 ビット)
Built from https://hg.mozilla.org/mozilla-central/rev/505bafeafb66b0083ce1fca8eec2d061f1a5ebb7

Broken
60.0a1 (2018-03-01) (64 ビット)
Built from https://hg.mozilla.org/mozilla-central/rev/0f8f71b0b9d84e7732c07f841e395de516b31b66

P.S. 2018-02-28-22-01-15-mozilla-central-l10n is broken (bug 1442145)
Flags: needinfo?(francesco.lodolo)
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=505bafeafb66b0083ce1fca8eec2d061f1a5ebb7&tochange=0f8f71b0b9d84e7732c07f841e395de516b31b66

A notable bug seems to be bug 1435912. But that's the bug that introduces Fluent strings in the General pane, so it might be just triggering an issue that it's already there.

> But, if I open the Console before Preferences, then it starts working. And I
> see a bunch of warnings related to missing strings. 
> 
> Can it be caused by the log functions? It would also explain why Italian
> works (no missing strings).
I can't reproduce it on Nightly, mac when trying to use a `ja` language pack. Flod - can you confirm?

STR:

1) Install Nightly
2) Install latest `ja` langpack for 61 from https://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central-l10n/win64/xpi/
3) In Browser Console set `Services.locale.setRequestedLocales(["ja"]);`
4) Restart browser
5) Open Preferences
6) cmd+t

Current Result:

Opens new tab
Flags: needinfo?(gandalf) → needinfo?(francesco.lodolo)
Hmm, I can't also reproduce steps in scenario 1 from comment 3 on MacOS ja-JP-mac Nightly.

Can someone else reproduce to reject the hypothesis that it's specific to Flod's setup?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> I can't reproduce it on Nightly, mac when trying to use a `ja` language
> pack. Flod - can you confirm?

Yes, I can replicate on a fresh profile with langpack installed (used intl.locale.requested in prefs since it was faster than remembering which key I need to change to get eval in Console)
Flags: needinfo?(francesco.lodolo)
For some reasons, it look harder to reproduce than this morning. Right now, I can get into this state more or less consistently by pressing cmd+, and then some keys while about:preferences is still loading.
(In reply to Francesco Lodolo [:flod] from comment #12)
> For some reasons, it look harder to reproduce than this morning. Right now,
> I can get into this state more or less consistently by pressing cmd+, and
> then some keys while about:preferences is still loading.

It happens consistently on 60.0b12 (64 ビット) (latest Japanese beta), also on a new profile. Japanese currently has bug 1450656, so not sure if that emphasizes a race condition.
Yep, please, don't use ja-JP-mac beta 60 for this testing until bug 1450656 gets uplifted.
I'd like to ask someone to reproduce it to find if it's a unique setup of Flod's machine that causes it or mine that doesn't reproduce it.

STR:

1) Download hebrew Firefox Nightly on Mac OS
2) Start on a fresh profile
3) Open Preferences
4) Press cmd+t

Expected Result:

New tab opens

Current Result:

Flod: Nothing happens
Zibi: New tab opens
Keywords: qawanted
Small correction.

3) Open preferences via keyboard shortcuts (cmd+,), while the page is loading press other keys on the keyboard (I use cursor keys)

Pressing any combination of keys with cmd (cmd+T, cmd+Q) focus the search field. If you close the windows using the mouse, you end up without menus on top, only "Firefox Nightly".
I get gandalf's result - with the 2018-04-16 Nightly he build on MacOS, I can't reproduce the primary bug.
Updated STR:

1) Download Hebrew Firefox Nightly on Mac OS
  https://download.mozilla.org/?product=firefox-nightly-latest-l10n-ssl&os=osx&lang=he

2) Start on a fresh profile

3) Press cmd+, to open Preferences

4) While Preferences are loading, press any key on the keyboard (e.g. cursor key)

5) After Preferences loaded, the search field is focused.

6) Press cmd+T

Expected Result:
New tab opens

Current Result:
Flod: Nothing happens
Zibi: New tab opens

I'm looking for someone to reproduce Flod's experience.
Abe - can you help us verify this?
Flags: needinfo?(amasresha)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #19)
> Abe - can you help us verify this?

Using the steps provided (in comment 18), I get a new tab. 

Test platform: 
OSX 10.13.4
MacBook Pro (Retina, 13-inch, Late 2013)
Graphics: Intel Iris 1536 MB
Flags: needinfo?(amasresha)
Thank you Abe!

Flod: what is the hardware/macos version you are using? Abe's and mine are both 2013 machines.
Status: NEW → UNCONFIRMED
Ever confirmed: false
I quite disagree that this bug is UNCONFIRMED, given that I can replicate on two completely different systems.

One is a 2010 iMac with a mechanical disk and an after-marked SSD, the other is a 2016 MB Pro 13, so disk performance is not really comparable between the two.

Model: iMac (27-inch, 2010)
CPU: 2,8 GHz Intel Core i5 
Memory: 12 GB
GPU: ATI Radeon HD 5750 1024 MB

Model: MacBook Pro (13-inch, 2016)
CPU: 3,3 GHz Intel Core i7
Memory: 16 GB
GPU: Intel Iris Graphics 550 1536 MB

They both run 10.13.4 (17E199) in Italian. Are you both running macOS in English?
I used unconfirmed to mark that we are looking for others to confirm. The fact that you reproduce on two machines disqualifies some hypothesis but not others. Since both machines are yours it's much more likely that the root cause is unique to your setup than if we can find another person with their setup reproduce it and then find what's common between those setups while unique compared to mine, Abe's or Mike's.
(In reply to Francesco Lodolo [:flod] from comment #22)
> They both run 10.13.4 (17E199) in Italian. Are you both running macOS in
> English?

Switched to English, including regional settings, rebooted and I still can reproduce.

And just to make sure we're all on the same page.

> 3) Press cmd+, to open Preferences
> 4) While Preferences are loading, press any key on the keyboard (e.g. cursor key)

#4 needs to happen while the page is still loading, which means you have to press cmd+, and cursor keys in one tight sequence.

A few other things that I noticed:
* The list of fonts takes a while to load the first time (the select remains empty while it's being populated, then switches to Arial). I only have 16 user fonts installed, so I don't see how that could be a problem.
* When I press cmd+T, etc. the search field is focused, even if I see the menu bar flashing (like it was trying to call the right command).
I can reproduce this. It's way funky, and there are no traces to my eyes as to why that happens.

I need a fresh start of the browser. Not necessarily a fresh profile. Then early enough, type something, and things get stuck. Only on that preference page, though.

- Error console doesn't show anything
- You can switch to different tabs, they work
- Go back to pref tab, still broken
- When focus is in the devtools bar on that page, keyboard shortcuts work

One thing that just dawns on me, I press keys before the search box is shown, which means probably that XBL bindings aren't attached yet, but the DOM element is already there. Maybe?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Also confirmed on http://archive.mozilla.org/pub/firefox/candidates/60.0b8-candidates/build1/mac/de/.

Note, you need to be slow enough for the content to load, and quick enough for layout to not kick in.
Thanks Axel!
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
I was able to reproduce it on Windows in Nightly (pl), but not beta or release.

It may be because on beta (pl) doesn't trigger any console warnings.

I'll try to get a build with a patch that would delay console.warnings from Fluent until after localization is done. If it fixes things on nightly it should fix it on beta as well.
I spent some time testing various hypothesis around this today and I think I found out a reliable way to reproduce it, and to make it not happen, which, if confirmed will at least zero down the culprit.

In order to reproduce this bug you need to launch a partially-localized build, so none of your regular en-US or it, but `pl` or `de` will work (at the moment, it can change as the localizers catch up).

Then you need to open Preferences and mash buttons as it opens, and if you're doin it right, once the page loads you won't be able to press "cmd+t" to open new tab.

Now, on to how to make it disappear:

1) Open the browser on which you reproduced the bug
2) Open browser console (cmd+j on mac)
3) Enter `Services.locale.setAvailableLocales(["pl"]);`
4) Then open Preferences and mash the buttons just the way you like
5) Press "cmd+t" and if I'm not mistaken a new tab is open

flod: can you verify the steps to make it not reproduce?
Flags: needinfo?(francesco.lodolo)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #29)
> flod: can you verify the steps to make it not reproduce?

I can confirm using Hebrew.

* I can safely reproduce the bug (3/3 tries) if I open Preferences first.
* Opening the console alone is not enough (correction to comment 2).
* If I run Services.locale.setAvailableLocales(["he"]); in Console before opening Prefs, everything works normally (3/3 tries).
Flags: needinfo?(francesco.lodolo)
Great!

I also tested if it's possible that just opening the console fixes it (which would indicate that initialization of console removes the issue) and tested if commenting out the console.warn we use fixes it. Neither does.

That means, in my opinion that the race condition is present because what `setAvailableLocales` does is that it makes us not fallback on another locale.

Here's how the process work in case of a complete localization:

1) Trigger opening Preferences
2) Preferences start loading and reach the Layout point
3) That triggers "MozBeforeInitialXULLayout" event which in turn triggers localization
4) C++ Node.localize collects localizable elements and triggers JS callback
5) JS callback collects l10n values and sends them back to C++
6) C++ localizes the content
7) Loading continus
8) Happy ending

Now, when we have incomplete locale, step (5) is more complicated:

5a) Collect l10n values for the main locale
5b) Notice missing values for some entries
5c) Trigger async I/O for the fallback locale
5d) Parse/format fallback locale and retrieve the remaining values
5e) Send combined values back to C++

This async I/O I suspect causes some shift in who wins the race condition around key bindings registration. I don't know why, but I strongly suspect that the race condition has always been there, it's just that we never had a fallback l10n so it was very hard or impossible to reproduce unless you were on a very, very slow hardware.

Based on this, I suggest that:

1) Reasses the priority of the bug. It requires quite specific combination of factors including incomplete locale and pressing keys while Preferences are loading to trigger. While I want to fix it, I'm not sure if it deserves P1.
2) Involve DOM Team engineers to help us debug it.
3) If it's not fixable in XBL, consider remodelling Fluent DOM integration to submit the strings available in (5b) before moving on to (5c).

But quite frankly, I'd prefer to fix it by finding out why pressing a key before preferences.xul <key> elements get localized messes with cmd+t/cmd+q bindings.

Setting NI on :jaws to help me asses the severity and next steps.
Flags: needinfo?(jaws)
Is https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/browser/components/preferences/in-content/preferences.xul#179 the only <key/> that is referenced by the preferences (not including subdialogs)? Would adding the <keyset/> and <key/> elements dynamically (after some point of loading) also fix this problem?
Flags: needinfo?(jaws)
Yep! That's brilliant, thank you Jared!
That fixes the Preferences UI, but we use the same logic in subdialogs, and I'd like to prevent having to risk someone somewhere not setting it since discovering it is hard.

So I filed bug 1455379 to fix it in XUL. Yikes. I know.
Comment on attachment 8969355 [details]
Bug 1454323 - Set the default key in <key> to avoid it capturing all command keys.

https://reviewboard.mozilla.org/r/238096/#review243820

Great!
Attachment #8969355 - Flags: review?(jaws) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/96867a07e1a3
Set the default key in <key> to avoid it capturing all command keys. r=jaws
https://hg.mozilla.org/mozilla-central/rev/96867a07e1a3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
I can't replicate anymore in 61.0a1 (2018-04-19). Setting NI for the uplift request.
Status: RESOLVED → VERIFIED
Flags: needinfo?(gandalf)
Comment on attachment 8969355 [details]
Bug 1454323 - Set the default key in <key> to avoid it capturing all command keys.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1424682
[User impact if declined]: it's possible for incomplete locale to lock command keys if keys are pressed during Preferences loading.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: Because it's a one line trivial change that has been eyeballed by enough experienced engineers and verified by the reporter to trust that we're not going to introduce any side regressions out of it.
[String changes made/needed]: none
Flags: needinfo?(gandalf)
Attachment #8969355 - Flags: approval-mozilla-beta?
Comment on attachment 8969355 [details]
Bug 1454323 - Set the default key in <key> to avoid it capturing all command keys.

fix a regression from using fluent in preferences, approved for 60.0b15
Attachment #8969355 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.