1389739 - ALTGR+Enter opens a canonized URL rather than opening in a new tab

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Eugene Savitsky]

This regression started in FF57.

1. I want to open www.menelon.ee. 
2. I enter "mene"
3. Autocomplete suggest www.menelon.ee. It is OK 
4. If I press Alt-Enter to open it in new tab, FF opens mene.com, instead of www.menelon.ee. 

Video: https://goo.gl/photos/VymQPspVjFy6Zutk6

Comment 1

[Marco Bonardo [:mak]]

The left ALT works properly, the right ALT (AltGr) acts like CTRL+ALT indeed, and thus it seems to activate canonize url (CTRL+Enter).

The code is here
http://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/browser/base/content/urlbarBindings.xml#809
We should check !event.altKey

Comment 2

[Jim Mathies [:jimm]]

Unassigned P1 tracking 57. Needs an owner or lower priority.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Talked to Jim offline, we don't assign all P1s in the search team, but this is still high priority for us. Brindusa, could we get some QA help in identifying the regressing change?

Comment 4

[Eugene Savitsky]

Actually it happens with FF56 as well.

Comment 5

[Eugene Savitsky]

Any ETA? 20 days till FF 56 release... Still Unassigned.

This is a major usability issue for a release.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Eugene Savitsky from comment #5)
> Any ETA? 20 days till FF 56 release... Still Unassigned.

Comment 0 states this started in 57, what's up with 56?

Btw, as I said the fix should be easy, we should just check if alt is pressed, and if so bail out of maybeCanonizeURL. Writing a test is likely taking more time than fixing the bug.

Comment 7

[Eugene Savitsky]

Actually it happens with FF56 as well.

Comment 8

[Eugene Savitsky]

But it happens not in 100% cases in FF56.

Comment 9

[Eugene Savitsky]

FF56: 
1. want to open https://www.ox.ee

2. typing in the URL bar: ox

3. FF shows https://www.ox.ee

4. AltGr+Enter = www.ox.com

Putting ox. brings to the www.ox.ee page, the desired one.

Comment 10

[Valentina Claudia Ona]

I did a regression range and here are the results: 
Last good revision: f08e729df988d122a6fa0cfea61b207acc101b89
First bad revision: 84f8a3442301c6e46aee261823d23fb1d2779a3e
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f08e729df988d122a6fa0cfea61b207acc101b89&tochange=84f8a3442301c6e46aee261823d23fb1d2779a3e

Comment 12

[Eugene Savitsky]

Any news here? Updated to FF56 and it happens now on a daily basis...

Comment 13

[Dion Williams [:dionrhys]]

I'm encountering this problem as well. I used to be able to:

1. Type "git" in the address bar
2. Press AltGr+Enter to open an autocompleted "https://github.com" address

Now it takes me to "www.git.com" instead.

Being able to use AltGr+Enter for the autocompleted entries lets me use one hand to navigate, as opposed to the two otherwise required for Alt+Enter.

Comment 14

[Marco Bonardo [:mak]]

Hopefully we can fix this for 58, it sounds annoying for who is used to that combo.

Comment 15

[Dion Williams [:dionrhys]]

(In reply to Marco Bonardo [::mak] from comment #1)
> The left ALT works properly, the right ALT (AltGr) acts like CTRL+ALT
> indeed, and thus it seems to activate canonize url (CTRL+Enter).
> 
> The code is here
> http://searchfox.org/mozilla-central/rev/
> 6482c8a5fa5c7446e82ef187d1a1faff49e3379e/browser/base/content/urlbarBindings.
> xml#809
> We should check !event.altKey

I'm not sure if just checking !event.altKey may be the best here.

I think the canonicalization should still happen when explicitly using Ctrl + Alt + Enter (canonicalizes and opens in new tab), likely enforcing the same behaviour with Ctrl + AltGr + Enter as well. I've never used the Ctrl canonicalization feature myself though, so haven't tested previous behaviour here.

Is there a way to distinguish whether an *actual* Ctrl key is being held for the canonicalization check, as opposed to the "fake" Ctrl and Alt keys that AltGr masquerades as?

Comment 16

[Marco Bonardo [:mak]]

(In reply to Dion Williams from comment #15)
> Is there a way to distinguish whether an *actual* Ctrl key is being held for
> the canonicalization check, as opposed to the "fake" Ctrl and Alt keys that
> AltGr masquerades as?

Afaik, on Windows it is not possible to differentiate, it just treats AltGr as CTRL+ALT, because on a keyboard where you miss ALtGr, you can do it with CTRL+ALT.

So, we must decide what we want it to do:
1. we can leave things as they are, users will be surprised by AltGr doing canonization instead of opening in new tab
2. we can check for altKey and disallow canonize and open in new tab at the same time (on Windows)
3. other solution currently unavailable to detect if AltGr is really the pressed key

Comment 17

[Dion Williams [:dionrhys]]

(In reply to Marco Bonardo [::mak] from comment #16)
> So, we must decide what we want it to do:
> 1. we can leave things as they are, users will be surprised by AltGr doing
> canonization instead of opening in new tab
> 2. we can check for altKey and disallow canonize and open in new tab at the
> same time (on Windows)
> 3. other solution currently unavailable to detect if AltGr is really the
> pressed key

I think Option 2 is the best right now then, if the OS limitations are as you say!

Comment 18

[Marco Bonardo [:mak]]

We must verify exactly the situation on Windows, Linux and Mac, also verify which of them can technically distinguish AltGR from CTRL+ALT.
We must also verify which was the behavior before the regression on those platforms. We could just decide to undo the regression.
If then there's still doubts, we can involve UX.
I set this as a P1 because it looked like a low hanging fruit, changing to P2 for now since it's still a not so common case and is not critical for a 58 release. Would be nice to have though.

Comment 19

[Eugene Savitsky]

On Windows AltGr always opened suggested URL in new tab. 

Now it is a mess that affects productivity. In 50% on usage I get some ridiculous ox.com, part.com etc...

Comment 20

[Marco Bonardo [:mak]]

Could be worth waiting for bug 900750 and see how events will change first.

Comment 21

[Eugene Savitsky]

I'm really ready to move to another browser now... It waists so much time and nerves every day.

Even "map..." sudgests maps.google.com, but opens map.com. And in English keyboard layout it is fine, only other languages are affected.

Comment 22

[Marco Bonardo [:mak]]

I'm sorry for your troubles, unfortunately this is not a top priority and there's an underlying change that is being discussed across browsers right now, we have to wait a little bit.

Comment 23

[Marco Bonardo [:mak]]

Windows seems to be working correctly now in Nightly, when using AltGr+Enter (likely because of bug 900750)

We may still want to add a test though.

Comment 24

[Eugene Savitsky]

Downloaded nightly 20180718. It does not work as it used to work.

English keyboard layout - OK, works fine.
Estonia keyboard layout - it opens the desired URL in same tab, instead of new one.


Year 2017: 
1. I want to open www.menelon.ee. 
2. I enter "mene"
3. Autocomplete suggest www.menelon.ee. It is OK 
4. If I press Alt-Enter to open it in new tab, FF opens www.menelon.ee. 

Year 2018:
1. I want to open www.menelon.ee. 
2. I enter "mene"
3. Autocomplete suggest www.menelon.ee. It is OK 
4. If I press Alt-Enter to open it in new tab, FF opens mene.com

Nightly 20180718:
1. I want to open www.menelon.ee. 
2. I enter "mene"
3. Autocomplete suggest www.menelon.ee. It is OK 
4. If I press Alt-Enter to open it in new tab, FF opens www.menelon.ee in same tab

Comment 25

[Marco Bonardo [:mak]]

At least we're no more sending users to unexpected origins.
On the italian and english layouts it works fine. Does the estonian layout have AltGraph or Right Alt?

We may want to change https://searchfox.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#705 to also handle AltGraph

Comment 26

[Eugene Savitsky]

How can I know the difference between AltGraph or Right Alt?

Maybe this helps? In Estonian Pressing:
AltGr + E = € sign
AltGr+ 2 = @ sign 

BTW In Russian layout it does not work as well. Same as Estonian.

Comment 27

[Eugene Savitsky]

Actually now it is a dataloss, since there is no way AltGr opens a new tab. It just opens the URL in current tab. In for example I've already lost Adwords campaign settings.

Comment 28

[Eugene Savitsky]

I filled:
https://bugzilla.mozilla.org/show_bug.cgi?id=1487632

Comment 30

[Eugene Savitsky]

Here's a very simple fix for this issue. Maybe it could be checked in to FF tree?

http://forums.mozillazine.org/viewtopic.php?f=23&t=3041472&p=14809159#p14809159

[*] Unzip omni.ja to a temporary directory
[*] Change the `_whereToOpen` function at line ~719 `chrome/browser/content/browser/urlbarBindings.xml` as follows:

Code: Select all
    let altEnter = event && (event.altKey || event.getModifierState("AltGraph"));


[*] Re-zip the omni.ja, place it back and start the browser with the `-purgecaches` argument to make sure the old bindings aren't cached anywhere (not sure if this is still needed).

Comment 31

[Eugene Savitsky]

Update: it does not work for search bar. I believe it should be changed in some other file.

Comment 32

[Eugene Savitsky]

Can someone check the fix in? Please...

Here are fixes for both: urlbar and searchbar available:
http://forums.mozillazine.org/viewtopic.php?f=23&t=3041472&p=14809159#p14809159

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late to fix in 63, but we can still take a patch for 65 or potentially 64.
Marking this as a possible [good first bug] since we have a suggested fix.

Comment 34

[Eugene Savitsky]

When someone fixes this, I would contribute 25 € to Mozilla foundation. I really suffer from this... :(

Comment 35

[Johannes Pfrang [:johnp]]

Attachment: bug1389739.patch

So I finally got to check-out mozilla-unified and create a patch.

I haven't been able to write a test due to two issues:

(1) Creating a KeyboardEvent with AltGraph set doesn't seem to be possible since KeyboardEventInitOptional doesn't accept "altGraphKey"
(2) While I was able to find `browser_urlbar_whereToOpen.js` for urlbar test, I couldn't find the whereToOpen test for the searchbar.


I checked the usages of `.altKey` in urlbar/searchbar and found two others that I didn't change: Alt+VK_DOWN/UP and `browser.altClickSave`. I don't know if it makes sense to support AltGraph for these use cases as well.

With the quantumbar rewrite all these uses and any additional uses of `.altKey`/`getModifierState("Alt")` should be considered as well, so we don't break stuff again and have good functionality for AltGraph layouts. I assume one should contact :dao for this?


This should also fix bug 1391239.

Comment 36

[Marco Bonardo [:mak]]

Thanks, I already had a version of this patch locally (also fixing the Quantum Bar code), but I'm indeed having problems simulating the AltGraph key in our test harness, for which I sent an e-mail today to get some hints about it. I'll report when I hear back about that.

There are a couple tests checking Alt for search too, but again if there's no easy way to simulate AltGraph it's a bit pointless.

Comment 37

[Johannes Pfrang [:johnp]]

Ok, thanks for the update. On second thought, I'm also unsure as to whether or not this fixes bug 1391239, since X11 seems to have some issue with the AltGr state getting stuck when changing (some) layouts (e.g. AltGr Compose / AltGr Unicode Combining) and that likely influenced my testing.

Comment 38

[h1121454]

In Windows, it is standard for AltGr to behave as Ctrl+Alt in most or all applications. In Firefox Ctrl canonizes URL while Alt opens a new tab.

Therefore the following is logical:

Ctrl+Alt = Canonize URL + open in new tab (works)
AltGR = Canonize URL + open in new tab (used to work, BROKEN in 63.0)

AltGR is really convenient because it's located at right side of keyboard, and in cases where you only need to open new tab and not canonize URL, you used to be able to type "example.org" and then AltGR+Enter and it did not canonize URL -- it only opened in new tab, which is the expected and convenient behavior.

However the original bug reporter has a problem with both the old version and the current version.

In order to fix both problems easily I suggest doing both the following: 

1. AltGR+Enter should both canonize and open new tab
2. In cases where URL has been autocompleted by the browser, AltGR should simply open new tab

This fixes both the original reporter's problem as mine.

Comment 39

[Liz Henry (:lizzard) (relman/hg->git project)]

Getting a bit late for 64, but we can still take a patch for 65.

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking fix-optional for 65 and 66 so that these already triaged issues don't show up repeatedly in weekly regression triage. Happy to take a patch in nightly.

Comment 41

[Eugene Savitsky]

So, not in FF65?

Comment 42

[Johannes Pfrang [:johnp]]

Doesn't look like it :/

mak, after checking the spec I've found a way that allows setting the modifier state[1], but I don't know if it works the same way in the test harness:

(new KeyboardEvent("", {code: "Enter", key: "Enter", modifierAltGraph: true})).getModifierState("AltGraph")

Since you already have a patch locally and I won't have the time to figure out the test harness for a while I'm unassigning myself.

[1] https://w3c.github.io/uievents/#event-constructors

Comment 44

[Eugene Savitsky]

I repeat my proposal:
When someone fixes this, I would contribute 25 € to Mozilla foundation. I really suffer from this... :(

FF66 and still not fixed. :(

Comment 45

[Eugene Savitsky]

Please... :((( It is not really hard to fix. The fix is here:

Here's a very simple fix for this issue. Maybe it could be checked in to FF tree?

http://forums.mozillazine.org/viewtopic.php?f=23&t=3041472&p=14809159#p14809159

[] Unzip omni.ja to a temporary directory
[] Change the _whereToOpen function at line ~719 chrome/browser/content/browser/urlbarBindings.xml as follows:

Code: Select all
let altEnter = event && (event.altKey || event.getModifierState("AltGraph"));

[*] Re-zip the omni.ja, place it back and start the browser with the -purgecaches argument to make sure the old bindings aren't cached anywhere (not sure if this is still needed).

Comment 46

[Bhopesh Bassi]

Is someone working on this? If not, I can take this. This is my first bug, so I might need some help with this.

Comment 47

[Eugene Savitsky]

Please take it...

Comment 48

[Bhopesh Bassi]

Is it okay if I email you with specific questions about the bug?

Comment 49

[Bhopesh Bassi]

I am using Firefox 68.0.1 on Windows 10, and I am not able to reproduce this bug. It works fine for me.

Comment 50

[Eugene Savitsky]

It does not work. Your keyb layout must be set to not English.

English keyboard layout - OK, works fine.
Estonian (Russian, German etc) keyboard layout - it opens the desired URL in same tab, instead of new one.

Here is the fix:

http://forums.mozillazine.org/viewtopic.php?f=23&t=3041472&p=14809159#p14809159

[] Unzip omni.ja to a temporary directory
[] Change the _whereToOpen function at line ~719 chrome/browser/content/browser/urlbarBindings.xml as follows:

Code: Select all
let altEnter = event && (event.altKey || event.getModifierState("AltGraph"));

[*] Re-zip the omni.ja, place it back and start the browser with the -purgecaches argument to make sure the old bindings aren't cached anywhere (not sure if this is still needed).

Comment 51

[Daniel Bodea [:danibodea]]

@Eugene: I have just checked whether this issue was fixed in the latest Nightly. I verified the functionality using an RO language build.

I only assume that Bhopesh Bassihas already fixed it. Can you confirm this?

Comment 52

[Bhopesh Bassi]

I haven't fixed it yet. I wasn't able to reproduce it earlier but after trying out different layouts, I can finally reproduce this.

Comment 53

[Bhopesh Bassi]

@danibodea It is not fixed on the latest nightly. I can reproduce it there.

Comment 54

[Drew Willcoxon :adw]

Bhopesh, thanks for volunteering to work on this. The code has changed a lot since the comments above. I think all you need to do now is change this line: https://searchfox.org/mozilla-central/rev/40e889be8ff926e32f7567957f4c316f14f6fbef/browser/components/urlbar/UrlbarInput.jsm#1282

To this:

if (
  !isMouseEvent &&
  event &&
  (event.altKey || event.getModifierState("AltGraph"))
) {

Comment 55

[Drew Willcoxon :adw]

Also, there are a few tests related to the alt key. You should run them and make sure they pass with your change. You should also add cases for testing AltGraph.

https://searchfox.org/mozilla-central/source/browser/components/urlbar/tests/browser/browser_urlbar_whereToOpen.js
https://searchfox.org/mozilla-central/source/browser/components/urlbar/tests/browser/browser_locationBarCommand.js
https://searchfox.org/mozilla-central/source/browser/components/urlbar/tests/browser/browser_urlbarEnter.js

Let us know here in the bug when you have questions.

Comment 56

[Bhopesh Bassi]

Hi Drew, Thanks a lot for this info. I will do as suggested by you.

I was looking at my local installation of Firefox and I saw that there is an xml file, urlbarBindings.xml, with the same function, _whereToOpen, as in UrlbarInput.jsm linked above. This file is located in zip file C:\Program Files\Mozilla Firefox\browser\omni.ja. Path inside omni.ja is chrome\browser\content\browser\urlbarBindings.xml. Can you please give some details on role of this xml file?

Comment 57

[Drew Willcoxon :adw]

urlbarBindings.xml is a file that we stopped using in Firefox 68 (and removed in 70). So if you make the change there, it won't appear to have any effect in 68 and later. In 68, we rewrote the urlbar, and UrlbarInput.jsm is one of the files of the new implementation.

You'll need to set up a development environment on your computer in order to make and submit your patch and run tests. Please see the documentation here for more info: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide You might start at the Getting Started link.

Comment 58

[Bhopesh Bassi]

I see. Thank you. Working on it.

Comment 59

[Bhopesh Bassi]

Hi Drew, I have made the change and it fixed the issue. I have some questions on writing and running new tests.

• How do I simulate AltGR key press? Online search suggests that both alt and ctrl key should be set to true while creating new KeyboardEvent. Is that correct?
• In this file, should I file altGrReturnKeypress as new function or just add test to the existing altReturnKeypress function? https://searchfox.org/mozilla-central/source/browser/components/urlbar/tests/browser/browser_urlbarEnter.js
• Does build run tests by default? I guess not. If no, then should I just run "./mach firefox-ui-functional" ?

Comment 60

[Marco Bonardo [:mak]]

I should have posted this sooner, so let me do that now, it's useful information we should not lose

On 2018/10/19 22:22, Marco Bonardo wrote:
Hello Masayuki-san,
I have a few questions regarding AltGraph.

I'm trying to write a js test generating a KeyboardEvent that has
getModifierState("AltGraph") set. I've tried various methods but I seem
unable to generate such an event. Do you have any suggestions on how to
do that?

If you'd like to synthesize in mochitests, you need to do as:

synthesizeKey("]", { altGrKey: true, code: "BracketRight", keyCode: 171});

This emulates '+' key of Spanish keyboard layout on Windows.
Unfortunately, we need to specify .code value and .keyCode value
explicitly for emulating non-US keyboard layout's specific key.

If you need to create untrusted event, you can create is as:

let keydownEvent = new KeyboardEvent("keydown", { key: "]", code:
"BracketRight", keyCode: 171, which: 171, modifierAltGraph: true });
let keypressEvent = new KeyboardEvent("keypress", { key: "]", code:
"BracketRight", charCode: 93, which: 93, modifierAltGraph: true });
let keyupEvent = new KeyboardEvent("keyup", { key: "]", code:
"BracketRight", keyCode: 171, which: 171, modifierAltGraph: true });

You can check proper value with this test tool:
https://w3c.github.io/uievents/tools/key-event-viewer.html

How does the event change on Linux and Mac? If I understood correctly on
Windows CTRL and ATL modifiers are false when AltGraph modifier is set,
what about the other OSes?

On Linux, it's really complicated. Roughly speaking, if a modifier key
is mapped to GDK_ISO_Level3_Shift (this is typically mapped to AltRight
key with some Europe keyboard languages), it activates "AltGraph" state
of each event.

On macOS, option key activates "AltGraph" state (with "Alt" state).
Although I don't know how other browsers treat it on macOS.

Comment 61

[Daniel Bodea [:danibodea]]

(In reply to Marco Bonardo [::mak] from comment #36)

Thanks, I already had a version of this patch locally (also fixing the
Quantum Bar code), but I'm indeed having problems simulating the AltGraph
key in our test harness, for which I sent an e-mail today to get some hints
about it. I'll report when I hear back about that.

I am having quite a hard time reproducing this issue now. Do you have any other tips on how to reproduce it or other tips that can help my manual testing?

These are my steps, taken from the comments above:

1. Download and install the latest Nightly with RU language.
2. Open browser.
3. Access the "www.menelon.ee" page a few times so it appears as a suggestion.
4. Open "www.google.com" page.
5. While the Google tab is focused, click inside the address bar, and input "mene using the keyboard.
   Observe: The menelon.ee suggestion is displayed.
6. Tap "AltGr"+"Enter" keyboard buttons.
   Observe: The menelon.ee page is being opened in another tab. (works exactly like when used the left Alt button)

Please note that I have verified it on Romanian and English languages with no differences; Also, the test has been performed on a desktop with a keyboard that has "Alt Gr" key and on an HP ProBook 430 G1 (also having an alt gr key on its keyboard).

In conclusion, I am not sure I understood the issue or there is something different between out steps to reproduce. Opinions?

Comment 62

[Marco Bonardo [:mak]]

I can reproduce easily the fact it opens in the current tab, not in another tab, it may indeed depend on the OS and the keyboard layout. My system is Win10, all italian and italian layout.
I don't know what causes the different behavior.

Comment 63

[Drew Willcoxon :adw]

(In reply to Bhopesh Bassi from comment #59)

Hi Drew, I have made the change and it fixed the issue.

Great!

• How do I simulate AltGR key press? Online search suggests that both alt and ctrl key should be set to true while creating new KeyboardEvent. Is that correct?

Please see Marco's comment 60 (thanks Marco). The synthesizeKey key call described in that comment sounds like the way to go.

• In this file, should I file altGrReturnKeypress as new function or just add test to the existing altReturnKeypress function? https://searchfox.org/mozilla-central/source/browser/components/urlbar/tests/browser/browser_urlbarEnter.js

I would probably add a new function just for this.

• Does build run tests by default? I guess not. If no, then should I just run "./mach firefox-ui-functional" ?

mach build doesn't run tests, no. Those three tests are all browser chrome mochitests. Details here: https://developer.mozilla.org/en-US/docs/Mozilla/Browser_chrome_tests

You can run each by doing:

mach test <path to test>

You can also do mach mochitest (as the doc says), but mach test is used to run all the various kinds of tests and figures out that these files are mochitests.

Comment 64

[Bhopesh Bassi]

Hi Marco, I have few questions about your suggestions:

• Why are we simulating right bracket( "]" ) key? Where is it coming from?
• Why are we using 'charCode' and 'which'? Both of these are deprecated according to documentation here. https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/KeyboardEvent
• Where is this modifierAltGraph property coming from? I don't see it documentation of KeyboardEvent.

Comment 65

[Bhopesh Bassi]

Hi Bodea,

• I can reproduce it on German layout. Can you try that?
• Can you try it on regular Firefox instead of nightly build?
• How are you adding layouts to Windows 10? I am adding it at Windows level from settings and then switching between them using button on right bottom of taskbar.

Comment 66

[Marco Bonardo [:mak]]

(In reply to Bhopesh Bassi from comment #64)

Hi Marco, I have few questions about your suggestions:

• Why are we simulating right bracket( "]" ) key? Where is it coming from?

it was just an example that Masayuki-san made on that mail, it's not strictly related to our case.

• Why are we using 'charCode' and 'which'? Both of these are deprecated according to documentation here. https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/KeyboardEvent
• Where is this modifierAltGraph property coming from? I don't see it documentation of KeyboardEvent.

What is exposed to Firefox chrome is not necessarily exposed to the Web. Anyway, in our case we don't need an untrusted event, so that part likely doesn't apply to our case. I just copied the ful e-mail for simplicity.

Comment 67

[Bhopesh Bassi]

Hi Drew, I have added the tests in all three files that you suggested. New tests in browser_urlbar_whereToOpen.js are working but new tests in browser_locationBarCommand.js and browser_urlbarEnter.js are failing due to some timeout and reference errors. Can you please help figure this out? What is the best way to share my code and tests with you? Should I just commit?

Comment 68

[Drew Willcoxon :adw]

Please post the patch to phabricator, our code-review tool, as described here:

• https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_4_Get_your_code_reviewed
• https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Comment 69

[Bhopesh Bassi]

Attachment: Open in new tab with AltGr+Enter (Bug 1389739).

Before this change, AltGr+Enter wouldn't open url in new tab on non-English layouts on Windows. This happened because non-English layouts generated AltGr event on pressing right Alt key. Checking for this event in _whereToOpen function fixes the issue.

Comment 70

[Bhopesh Bassi]

Hi Drew, I have started a review request. Here is the url. https://phabricator.services.mozilla.com/D39466
I haven't removed the older code but just commented it. I will remove it when I update the patch after you take a look at failing tests.

I have few questions:

• I used arc but documentation referred to moz-phab as a better alternative. I couldn't find instructions on how to use moz-phab on Windows. Can you please point me to right documentation?
• I never did "hg push". I just did "hg commit" and then "arc diff". Is this how it is supposed to be? I am little confused because I come from git world and there the standard procedure is to commit on a new branch and then push that branch. Here, while following instructions, I committed on default branch and then never pushed to remote repo but did "arc diff".

Comment 71

[Marco Bonardo [:mak]]

./mach boostrap works on Windows and can setup most of your needs, IIRC included moz-phab

Comment 72

[Daniel Bodea [:danibodea]]

I finally understood the issue. It is not specific to builds of certain languages. The OS keyboard layout is the cause. What I mean is that the issue reproduces on any Firefox build, while the Windows' keyboard layout is set as any other than standard English. When the keyboard layout is set as Deutch or Russian (as an example), the Right Alt will be treated as Alt Graph rather than just like the Left Alt, hence the issue's appearance.

In Fx55:
The final result of the steps from comment 61 is that the "menelon.ee" page would be opened in a new tab; the expected/correct result.

I have found two changes related to this area:

1. Somewhere in Fx56, regressing the change gave me this: "2019-07-26T12:14:49: INFO : Narrowed nightly regression window from [2017-07-22, 2017-07-24] (2 days) to [2017-07-22, 2017-07-23] (1 days) (~0 steps left)" The bisection could not be finished due to lack of builds.
   With this change, the final result of the steps from comment 61 would be that the right Alt will be treated as Alt Graph (on Windows 10) so when the steps from comment 61 were performed, the final observation is that "meme.com" page would be opened in a new tab. (bad/wrong result)
2. Somewhere in Fx63 I found another change relating this area, regressing it gave me this: "2019-07-26T14:18:11: INFO : Narrowed nightly regression window from [2018-06-23, 2018-06-26] (3 days) to [2018-06-25, 2018-06-26] (1 days) (~0 steps left)" The bisection could not be finished due to lack of builds.
   With this change, the final result of the steps from comment 61 would be that the right Alt will be treated as Alt Graph (on Windows 10) so when the steps from comment 61 were performed, the final observation is that "memelon.ee" page would be opened in the same tab. (still bad/wrong result)

Hope this helps.

Comment 73

[Eugene Savitsky]

BTW Linux is affected as well.

Comment 74

[Bhopesh Bassi]

Yes Daniel, that is the correct issue. I wasn't aware that you were testing something else earlier.

Comment 75

[Bhopesh Bassi]

Can some please take my patch and run it once? I am having some issues with test cases and need help with that.

Comment 76

[Eugene Savitsky]

I cat test only full .exe version, I'm not a programmer...

Comment 77

[Bhopesh Bassi]

Then I will wait for Drew or Marco to run the tests once.

Comment 78

[Drew Willcoxon :adw]

I spent some time trying to figure out how to synthesize AltGraph. Masayuki's suggestions in comment 60 don't seem to work, but according to the date in that comment, they're 9 months old, so maybe something has changed. Or maybe I'm doing it wrong.

I did get EventUtils.synthesizeNativeKey to work. But synthesizeNativeKey only works on Windows and Mac, not Linux. So for the two failing tests, you can do something like this (untested), and let's just skip the AltGraph checks on Linux unfortunately.

// From EventUtils.js
const KEYBOARD_LAYOUT_SPANISH = {
  name: "Spanish",
  Mac: 12,
  Win: 0x0000040A,
  hasAltGrOnWin: true,
};

// From NativeKeyCodes.js
const WIN_VK_RETURN = 0x001C000D;
const MAC_VK_Return = 0x24;

let keyCode;
switch (AppConstants.platform) {
  case "win":
    keyCode = WIN_VK_RETURN;
    break;
  case "macosx":
    keyCode = MAC_VK_Return;
    break;
  default:
    // synthesizeNativeKey not supported, return or throw or something
}

EventUtils.synthesizeNativeKey(KEYBOARD_LAYOUT_SPANISH, keyCode,
                               { altGrKey: true }, "", "");

Comment 79

[Drew Willcoxon :adw]

(In reply to Bhopesh Bassi from comment #70)

• I used arc but documentation referred to moz-phab as a better alternative. I couldn't find instructions on how to use moz-phab on Windows. Can you please point me to right documentation?

In addition to what Marco said, there's some documentation here: https://github.com/mozilla-conduit/review/blob/master/README.md

• I never did "hg push". I just did "hg commit" and then "arc diff". Is this how it is supposed to be? I am little confused because I come from git world and there the standard procedure is to commit on a new branch and then push that branch. Here, while following instructions, I committed on default branch and then never pushed to remote repo but did "arc diff".

That's pretty much right. arc diff will complain if you try to make a diff with multiple commits (I think?) so you'll need to squash your commits or hg commit --amend. And moz-phab (I think?) can submit multiple commits, but it will submit each commit as a different revision, which is not what we want here. We want one revision that's updated as you address review comments.

Comment 80

[Bhopesh Bassi]

Hi Drew, Currently we are using EventUtils.synthesizeKey, do you want to use EventUtils.synthesizeNativeKey instead?

The code snippet you shared above will only test Spanish layout, what about other layouts? Shouldn't we try to keep tests layout independent?

What else can we try? Can you refer me to some documentation on testing framework? Tests for whereToOpen are using javascript KeyboardEvent, why can't we use same for other tests?

Comment 81

[Marco Bonardo [:mak]]

to merge multiple already existing changesets you can also use hg rebase -s source -d dest --collapse

For the test, the only thing we are trying to do is to generate a keyboard event having getModifierState("AltGraph") set, the way we generate it doesn't matter much, as well as the keybord layout, because we are basically unit testing our handling of the AltGraph modifier. We are not testing the platform code that generates the event, we are just testing the effect of an AltGraph event.

Comment 82

[Bhopesh Bassi]

But shouldn't we try to generate it in keyboard independent way? and why can't we use Javascript KeyboardEvent like we used for whereToOpen tests.

Comment 83

[Marco Bonardo [:mak]]

We can't test every combination, it would be too expensive, so we unit test the specific fix, that is handling getModifierState("AltGraph"), it doesn't really matter how we generate it provided it's reasonably close to a real user action.
I don't know the answer to your question, because that's in the scope of resolving this bug, it requires the assignee (or someone else) to investigate those possibilities, like for example Drew did for comment 78. I didn't have time to look into it yet.

Comment 84

[Drew Willcoxon :adw]

I guess we could just create KeyboardEvents and dispatch them to the input. That might work. I suggested synthesizeNativeKey because those two tests are already using synthesizeKey. If you want to create and dispatch KeyboardEvents in those tests instead, then you may have to work around the frameworks that those tests are already using, which is fine but a little more work.

Comment 85

[Bhopesh Bassi]

Looking at documentation of EventUtils.synthesizeKey, I figured that it expects {'altGraphKey' : true} instead of {'altGrKey' : true}. Please see this https://searchfox.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#837

After making this change, tests started working. I have updated the revision, please take a look once. Please also advise on next steps.

Comment 86

[Drew Willcoxon :adw]

Great, thanks! I've accepted your patch and pushed it to tryserver to make sure it passes our tests. If it does, then I'll commit it. The link to see the tryserver results is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e3cce1cfc0a54cfe585d537fd06dc77f40b4c20

I'd also like to ask Eugene to test it before committing, but we need to wait for tryserver to finish building it first.

Comment 87

[Eugene Savitsky]

I'm ready to test a build! Just give me a download URL.

Praying that it works...

Comment 88

[Drew Willcoxon :adw]

From your comments above, it sounds like you're using Windows? This is a Windows 10 x64 build: https://queue.taskcluster.net/v1/task/GG0zRK0YQBOJfRT1El8ohQ/runs/0/artifacts/public/build/target.zip

Please let me know if you need a build for a different platform.

Comment 89

[Eugene Savitsky]

It works just fine! Hurray!

Comment 90

[Eugene Savitsky]

BTW Tested with Russian and Estonian keyb language.

Comment 91

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df4af426149c
Open in new tab with AltGr+Enter . r=adw

Comment 92

[Drew Willcoxon :adw]

Thanks. I just landed this, so it should be on a Nightly tomorrow or the next day.

Thanks, Bhopesh!

Comment 93

[Bhopesh Bassi]

Hi Drew, I have few questions.

• I see some tests failing here for completely unrelated reason. What are these about? https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=1e3cce1cfc0a54cfe585d537fd06dc77f40b4c20
• What is the bug opened by Tara above? I don't completely get it.

Comment 94

[Drew Willcoxon :adw]

(In reply to Bhopesh Bassi from comment #93)

• I see some tests failing here for completely unrelated reason. What are these about? https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=1e3cce1cfc0a54cfe585d537fd06dc77f40b4c20

Unfortunately we have a lot of intermittent test failures. We open bugs for them so we can track them. Intermittent failures usually happen on every landing, but not the same failures.

• What is the bug opened by Tara above? I don't completely get it.

It looks like your patch might have caused a new intermittent failure, possibly specifically the browser_urlbarEnter.js addition. I really doubt it since that addition is not doing anything that the test already wasn't doing. I searched Bugzilla and found a couple of identical intermittent failure bugs, but they were both closed since they happened very rarely and not recently. (I marked those bugs as see-also's on the new bug.) So I would guess that there's something about this test that causes this failure very rarely. I don't immediately see what it could be. It's possible your patch made it more frequent, or maybe your push was just unlucky and it happened to occur. We can keep an eye on the bug to see if it keeps happening.

Comment 95

[Razvan Maries]

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

Comment 96

[Marco Bonardo [:mak]]

Thank you again for looking into this bug and patching it.

Comment 97

[Eugene Savitsky]

As promised, donated to Mozilla 25 €.

Tnx, Bhopesh Bassi !

Comment 98

[Eugene Savitsky]

In nightly it is fixed for URLbar, but not for searchbar.

Comment 99

[Mark Banner (:standard8)]

(In reply to Eugene Savitsky from comment #98)

In nightly it is fixed for URLbar, but not for searchbar.

Search bar is probably bug 1391239.

Comment 100

[Bhopesh Bassi]

Do we still have to different bars? I thought the same bar worked as url and search!!

Comment 101

[Drew Willcoxon :adw]

You can toggle the separate search bar in Firefox's preferences, in the Search tab.

Comment 102

[Eugene Savitsky]

I prefer the separate one. So the URL bar dropdown list is clean from the search suggestions.

Comment 103

[Daniel Bodea [:danibodea]]

I have verified this issue on Nighly v71.0a1 from 2019-09-15 and Beta v70.0b6 using e desktop and a laptop test machines (that have an Alt Gr key) with Windows 10 x64 and Windows 7 x32, with system keyboard input language set to Deutch. It does not reproduce on Nightly or Beta, but it reproduces in Release.
Thank you!

Comment 104

[Eugene Savitsky]

It is broken for Firefox Linux Wayland (works fine in X11 session).
https://bugzilla.mozilla.org/show_bug.cgi?id=1827615

Could someone take a look, please?