Closed Bug 1389739 Opened 7 years ago Closed 5 years ago

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

Categories

(Firefox :: Address Bar, defect, P2)

Product:
Firefox
Component:
Address Bar
Version:
57 Branch
Platform:
x86_64
Windows 10
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 70
Tracking Flags:
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- verified
firefox71 --- verified

People

(Reporter: ezh, Assigned: bbassi, Mentored)

References

(Depends on 1 open bug, Regressed 1 open bug)

Details

(Keywords: regression, Whiteboard: [fxsearch] [good first bug])

Attachments

(2 files)

bug1389739.patch
1.66 KB, patch
Details | Diff | Splinter Review
Open in new tab with AltGr+Enter (Bug 1389739).
47 bytes, text/x-phabricator-request
Details | Review 
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
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
Keywords: regression
Priority: -- → P1
Whiteboard: [fxsearch]
Unassigned P1 tracking 57. Needs an owner or lower priority.
Flags: needinfo?(past)
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?
Flags: needinfo?(past)
Keywords: regressionwindow-wanted
Actually it happens with FF56 as well.
Any ETA? 20 days till FF 56 release... Still Unassigned.

This is a major usability issue for a release.
(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.
Actually it happens with FF56 as well.
But it happens not in 100% cases in FF56.
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.
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
Depends on: 448486
Summary: Opening different URL, as autocomplete suggested → ALTGR+Enter opens a canonized URL rather than opening in a new tab
Any news here? Updated to FF56 and it happens now on a daily basis...
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.
Hopefully we can fix this for 58, it sounds annoying for who is used to that combo.
Priority: P2 → P1
(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?
(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
See Also: → 1391239
(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!
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.
Priority: P1 → P2
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...
Could be worth waiting for bug 900750 and see how events will change first.
Depends on: 900750
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.
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.
Depends on: 1444581
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.
Priority: P2 → P3
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
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
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.
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.
See Also: → 1487632
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).
Update: it does not work for search bar. I believe it should be changed in some other file.
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
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.
status-firefox63: affected → wontfix
status-firefox64: --- → affected
Whiteboard: [fxsearch] → [fxsearch] [good first bug]
When someone fixes this, I would contribute 25 € to Mozilla foundation. I really suffer from this... :(
Attached patch bug1389739.patch — — Splinter Review 
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.
Assignee: nobody → johnp
Status: NEW → ASSIGNED
Attachment #9018617 - Flags: review?(mak77)
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.
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.
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.
Getting a bit late for 64, but we can still take a patch for 65.
status-firefox64: affected → fix-optional
status-firefox65: --- → affected
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.
status-firefox65: affected → fix-optional
status-firefox66: --- → fix-optional

So, not in FF65?

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

Assignee: johnp → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mak77)
Attachment #9018617 - Flags: review?(mak77)
Depends on: quantumbar
Flags: needinfo?(mak77)

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. :(

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).

Priority: P3 → P2

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

Flags: needinfo?(ezh)

Please take it...

Flags: needinfo?(ezh)

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

Flags: needinfo?(ezh)

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

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).

Flags: needinfo?(ezh)

@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?

Flags: needinfo?(ezh)

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.

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

Flags: needinfo?(daniel.bodea)

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"))
) {
Flags: needinfo?(ezh)
Flags: needinfo?(daniel.bodea)
Assignee: nobody → bbassi
Mentor: adw

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?

Flags: needinfo?(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.

Flags: needinfo?(adw)

I see. Thank you. Working on it.

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

Flags: needinfo?(adw)

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.

(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?

Flags: needinfo?(ezh)
Flags: needinfo?(mak77)

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.

Flags: needinfo?(mak77)

(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.

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.

Flags: needinfo?(adw)

Hi Marco, I have few questions about your suggestions:

Flags: needinfo?(mak77)

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.

(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.

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.

Flags: needinfo?(mak77)

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?

Flags: needinfo?(adw)

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.

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".
Flags: needinfo?(adw)

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

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.

status-firefox68: --- → affected
status-firefox69: --- → affected
status-firefox70: --- → affected
Keywords: regressionwindow-wanted

BTW Linux is affected as well.

Flags: needinfo?(ezh)

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

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

Flags: needinfo?(ezh)

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

Flags: needinfo?(ezh)

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

Flags: needinfo?(mak77)

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 }, "", "");
Flags: needinfo?(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.

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?

Flags: needinfo?(adw)

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.

Flags: needinfo?(mak77)

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.

Flags: needinfo?(mak77)

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.

Flags: needinfo?(mak77)

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.

Flags: needinfo?(adw)

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.

Flags: needinfo?(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.

Flags: needinfo?(adw)

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

Praying that it works...

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.

Flags: needinfo?(ezh)

It works just fine! Hurray!

Flags: needinfo?(ezh)

BTW Tested with Russian and Estonian keyb language.

Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df4af426149c
Open in new tab with AltGr+Enter . r=adw

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

Thanks, Bhopesh!

Status: NEW → ASSIGNED

Hi Drew, I have few questions.

Flags: needinfo?(adw)

(In reply to Bhopesh Bassi from comment #93)

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.

Flags: needinfo?(adw)

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

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox70: fix-optional → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

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

As promised, donated to Mozilla 25 €.

Tnx, Bhopesh Bassi !

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

Flags: needinfo?(bbassi)

(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.

Flags: needinfo?(bbassi)

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

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

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

Flags: qe-verify+

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!

Status: RESOLVED → VERIFIED
status-firefox70: fixed → verified
status-firefox71: --- → verified
Flags: qe-verify+
Regressions: 1627502
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: