1086524 - pressing Esc when address bar is selected should return focus to window

Status: RESOLVED FIXED

(Firefox :: Address Bar, enhancement, P3)

Description

[Vittorio Giovara]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141011015303

Steps to reproduce:

I wanted to go back one page when the address bar was selected, but without using the mouse. So I pressed backspace and it didn't work, then I pressed cmd/alt and arrow keys but it didn't work either.


Actual results:

The address was cleared out in case of backspace, and the cursor went at the beginning or at the end of it.

My keyboard was imprisoned in the address bar and the only way to exit was with a few tabs. 


Expected results:

Ideally when pressing 'esc' when the address bar is selected (and there is no more undo to it) it should return priority to the window and allow to navigate history normally.

Comment 1

[YF (Yang)]

Press a several of Tab key may focus to page.

Comment 2

[[:Aleksej]]

I have no problem with Alt+Left on Linux even if the address bar is focused.  If I had, I'd use F6 to switch focus.

Comment 3

[Lindhe94]

A workaround is to use Tab to move focus from the address bar to the page.

Comment 4

[osbornejr]

This is still an issue. On a Mac the F6 key is very inconvenient (requires Fn key), and the Esc key just makes more sense-- it performs this function in every other pane or input box.

Comment 5

[Ronan Jouchet [:ronjouch]]

To [::dao] or anyone: I'd like to give this a stab.

Before I try to get my hands in the code, is there any good reason (UX? Backwards compatibility? Addons?) for not implementing this?
(And that would mean an eventual patch of mine would be refused 😕)

Comment 6

[dough.mean]

Still a problem in 111.0.1
I have my capslock key mapped to escape (just like many other vim users), so any other alternative key combination by comparison is just plain un-ergonomic.
I'm very much hoping that this very standard UX behavior will be implemented soon.

Comment 7

[flippidippi]

I recently switch from Chrome to Firefox and this is driving me crazy. Every other input box loses focus when pressing escape. It makes sense to do the same or at least give an option for this behavior in the address bar.

Comment 8

[flippidippi]

Attachment: Bug 1086524 - Focus window on Esc in address bar r=mak,adw,dao

When in the address bar and not a user typed value
Esc will focus the window

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 10

[Collin Richards]

Hi, I'm also wanting this feature. I actually implemented a fix for myself before logging in on bugzilla and realizing that a fix has already been implemented, but there are some outstanding revisions requested. I am not very familiar with the process of contributing, but if I address the comments in phabricator with a changeset is there hope for getting this functionality reviewed and added? Maybe the original contributor is still interested in completing the work, if not then I'm willing to take it over.

Comment 11

[Marco Bonardo [:mak]]

Yes, we'd accept patches, and addressing comments is a good start.
There's some contributor documentation at https://firefox-source-docs.mozilla.org/setup/contributing_code.html

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:dao, since the bug has recent activity, could you have a look please?

For more information, please visit BugBot documentation.

Comment 13

[Dão Gottwald [:dao]]

Hey Collin, looks like flippidippi is inactive, so this is good to pick up.

Comment 14

[Collin Richards]

Attachment: Bug 1086524 - Focus window on Esc in address bar r=dao,#urlbar-reviewers

Comment 15

[Collin Richards]

I addressed most of the issues in a new changeset. Please let me know if there are any other desired changes.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c052f2639595
Focus window on Esc in address bar r=dao,urlbar-reviewers

Comment 17

[Iulian Moraru]

Backed out for causing multiple browser failures related to urlbar.

• Push with failures

• Failure log for browser-a11y failures

• Failure log for browser-chrome failures

• Backout link

Comment 18

[Collin Richards]

The failing accessible/tests/browser/events/browser_test_selection_urlbar.js test relies on being able to press escape twice after window open to more reliably select the url text. Now pressing escape multiple times will result in the url text deselected and then the url bar unfocused. I could only reliably get this test to succeed by removing both of the calls to escape.

The failing tests in browser/components/urlbar/tests/browser/browser_UrlbarInput_searchTerms_revert.js are related to restoring logic with browser.urlbar.showSearchTerms.featureGate enabled. Changing the call from this.input.handleRevert(true) to this.input.handleRevert(false) in UrlbarController.sys.mjs:334 seemed to have the desired behavior when testing the url reset behavior manually, but don't cause the tests to pass.

I'll spend some more time this week investigating and trying to get these tests to pass.

Comment 19

[Dão Gottwald [:dao]]

Hey Collin, have you had the chance to take another look at getting those tests to pass? No pressure, just checking to make sure this bug doesn't get lost. Let us know if we can help. Thanks!

Comment 20

[Collin Richards]

No, I haven't had a chance to investigate much further then I did previously. Once I have a bit of free time I'll post another changeset.

For the failing accessibility test I'm tempted to remove the two calls to synthesize the escape key in accessible/tests/browser/events/browser_test_selection_urlbar.js, but the comments make me think they might have originally there to avoid flaky tests. Any thoughts on that?

Comment 21

[Dão Gottwald [:dao]]

(In reply to Collin Richards from comment #20)

No, I haven't had a chance to investigate much further then I did previously. Once I have a bit of free time I'll post another changeset.

For the failing accessibility test I'm tempted to remove the two calls to synthesize the escape key in accessible/tests/browser/events/browser_test_selection_urlbar.js, but the comments make me think they might have originally there to avoid flaky tests. Any thoughts on that?

I think that's fine. The test is a few years old and there's a chance that what was flaky isn't anymore. It passes for me locally with that change.

Regarding browser_UrlbarInput_searchTerms_revert.js, it looks like we might be hitting Esc too often in the local synthesizeRevert function. Removing { repeat: 2 } seems to make the test pass, but I haven't fully read through the test to see that it still tests what it's supposed to test.

Does this help?

Comment 22

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #21)

(In reply to Collin Richards from comment #20)

No, I haven't had a chance to investigate much further then I did previously. Once I have a bit of free time I'll post another changeset.

For the failing accessibility test I'm tempted to remove the two calls to synthesize the escape key in accessible/tests/browser/events/browser_test_selection_urlbar.js, but the comments make me think they might have originally there to avoid flaky tests. Any thoughts on that?

I think that's fine. The test is a few years old and there's a chance that what was flaky isn't anymore. It passes for me locally with that change.

Regarding browser_UrlbarInput_searchTerms_revert.js, it looks like we might be hitting Esc too often in the local synthesizeRevert function. Removing { repeat: 2 } seems to make the test pass, but I haven't fully read through the test to see that it still tests what it's supposed to test.

I updated the patch with these two test fixes, however there's still an issue with browser_ime_composition.js that I'm not sure how to resolve: https://treeherder.mozilla.org/jobs?repo=try&revision=cb55c3367a2b3ad48af354c95da299d4454dec12&selectedTaskRun=RN1RgNOFR_CXrfaaX1YaKw.0
Marco, do you have ideas here?

Comment 23

[Marco Bonardo [:mak]]

(In reply to Dão Gottwald [:dao] from comment #22)

I updated the patch with these two test fixes, however there's still an issue with browser_ime_composition.js that I'm not sure how to resolve: https://treeherder.mozilla.org/jobs?repo=try&revision=cb55c3367a2b3ad48af354c95da299d4454dec12&selectedTaskRun=RN1RgNOFR_CXrfaaX1YaKw.0
Marco, do you have ideas here?

Off-hand I would guess some of the ESC press before composeAndCheckPanel are excessive, the panel is already closed (we close the panel on compositionstart) and we press ESC thinking it's open, and then with this patch the urlbar loses focus and composition fails.
Maybe promisePopupClose should check the panel is effectively open before executing the closeFn?

Comment 24

[Dão Gottwald [:dao]]

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

Off-hand I would guess some of the ESC press before composeAndCheckPanel are excessive, the panel is already closed (we close the panel on compositionstart) and we press ESC thinking it's open, and then with this patch the urlbar loses focus and composition fails.

That's helpful, thanks. Wasn't sure about the implied panel states around composition.

Maybe promisePopupClose should check the panel is effectively open before executing the closeFn?

Yeah, I think it should probably report a failure in that case. However, to get this landed, I'll first try to fix browser_ime_composition.js specifically and see if that's sufficient, and if so we can revisit promisePopupClose in a separate bug.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c03123359b41
Focus window on Esc in address bar r=dao,urlbar-reviewers

Comment 26

[agoloman]

Backed out for causing

• Backout link
• Push with failures
• Failure Log
• Failure line:

[task 2024-10-16T12:56:34.416Z] 12:56:34     INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_persist_searchMode.js | Urlbar should never be in search mode without the corresponding attribute. - true == true - 
[task 2024-10-16T12:56:34.417Z] 12:56:34     INFO - Buffered messages finished
[task 2024-10-16T12:56:34.417Z] 12:56:34     INFO - TEST-UNEXPECTED-FAIL | browser/components/urlbar/tests/browser/browser_persist_searchMode.js | gURLBar.searchMode should exist as expected - true == false - 
[task 2024-10-16T12:56:34.417Z] 12:56:34     INFO - Stack trace:
[task 2024-10-16T12:56:34.417Z] 12:56:34     INFO - resource://testing-common/UrlbarTestUtils.sys.mjs:assertSearchMode:827
[task 2024-10-16T12:56:34.418Z] 12:56:34     INFO - chrome://mochitests/content/browser/browser/components/urlbar/tests/browser/browser_persist_searchMode.js:test_escape_searchmode:113
[task 2024-10-16T12:56:34.418Z] 12:56:34     INFO - chrome://mochikit/content/browser-test.js:handleTask:1145
[task 2024-10-16T12:56:34.418Z] 12:56:34     INFO - chrome://mochikit/content/browser-test.js:_runTaskBasedTest:1217
[task 2024-10-16T12:56:34.418Z] 12:56:34     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1358
[task 2024-10-16T12:56:34.418Z] 12:56:34     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1134
[task 2024-10-16T12:56:34.418Z] 12:56:34     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/<:1058

Please check the other failures in the log.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c3560df7c73f
Focus window on Esc in address bar r=dao,urlbar-reviewers

Comment 28

[Cosmin Sabou [:CosminS]]

Backed out changeset c3560df7c73f (Bug 1086524) for causing bc failures on browser_searchModeSwitcher_basic.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&test_paths=browser%2Fcomponents%2Furlbar%2Ftests%2Fbrowser&revision=c3560df7c73f0380a3a6e13c701bcb2818a16c88&selectedTaskRun=PqgrDBKgSYqLUKHhvQlG-w.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=478557188&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/fc5e94141008bf78b701d8458e0af55165b7b0d5

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91c817b56f61
Focus window on Esc in address bar r=dao,urlbar-reviewers

Comment 30

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/91c817b56f61