1435530 - Alt+D does not always work like Ctrl+L

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P2)

Description

[Antoine Potten]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180128191252

Steps to reproduce:

Since FF 57 (this was working with earlier versions) Alt+D does not work in all the cases as equivalent of Ctrl+L.
Tested only on Windows 10.
This is a particular setup, as I modified the following values in about:config
ui.key.chromeAccess=5 (instead of default 4)
ui.key.contentAccess=4 (instead of default 5)
This bug only applies when these values are set that way (to be able to use Alt without Shift for page's access keys).
I know this messes up the behaviour of "Alt+" shortcuts, but as said above it was working in pre-57 versions, and has anyway an inconsistent behaviour now.

Compare pressing Alt+D or Ctrl+L when the page has the focus vs. pressing these keys when the focus is already in the address bar or in the search bar.


Actual results:

In FF57 & 58, Alt+D focuses the address bar only if the page had the focus.
If the address bar was already focused but contents not selected, the whole contents is not selected (unlike with Ctrl+L).
If the search bar is focused, nothing happens.


Expected results:

In all cases, Alt+D should focus the address bar and select its contents, not only when the page has the focus. 
Ctrl+D works fine.

Comment 1

[Gingerbread Man]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
20180203100135

https://hg.mozilla.org/integration/autoland/json-pushes?changeset=57e0c8a166d2b378eed7c0008643ade6b184145f&full=1

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

It's declared as a *chrome* access key.
https://searchfox.org/mozilla-central/rev/e06af9c36a73a27864302cd2f829e6200dee8541/browser/locales/en-US/chrome/browser/browser.dtd#431

So, in the comment 0's settings, it should be Alt + Shift + D unless you set "ui.key.generalAccessKey" to 18.

Even before fixing bug 1333459, shift key IS compared strictly:
https://searchfox.org/mozilla-central/rev/a7be68f59ac68db365c6ee11201551117fb1c53e/dom/events/EventStateManager.cpp#768,773-774,777-778,786,788-789,793-794
https://searchfox.org/mozilla-central/rev/a7be68f59ac68db365c6ee11201551117fb1c53e/dom/events/EventStateManager.cpp#1125,1128,1131,1141,1143,1176-1178,1199-1201

So, I don't understand why Alt + D moves focus to the URL bar with such settings.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #2)
> It's declared as a *chrome* access key.
> https://searchfox.org/mozilla-central/rev/e06af9c36a73a27864302cd2f829e6200dee8541/browser/locales/en-US/chrome/browser/browser.dtd#431

Oh, it's not actually an accesskey.  It's defined as a shortcut key here:
https://searchfox.org/mozilla-central/rev/e06af9c36a73a27864302cd2f829e6200dee8541/browser/base/content/browser-sets.inc#199-202

This was added by bug 231381 and looks like it's dirty hack...

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dce057ba12c57d9d609f8476d3c24f3aab1d45ed

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=409f85c81b07320e5b664072ab2c49e4c87722ca

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1435530 - part 1: Add automated tests to check Alt + D works as Ctrl + L when Alt is content access key's modifier

Review commit: https://reviewboard.mozilla.org/r/217858/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217858/

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1435530 - part 2: Make nsXBLWindowKeyHandler treat eAccessKeyNotFound as eKeyPress event

Modifiers of shortcut keys may be same as modifier of content access keys.

When focus is in the main process, such eKeyPress event is sent to remote
content first.  Then, and if it's not handled in the remote content,
eAccessKeyNotFound is dispatched into the DOM tree in the main process.
However, nsXBLWindowKeyHandler doesn't handle it as eKeyPress event.  So,
it causes that shortcut keys whose modifier conflicts with content access key
won't be handled.

This patch just makes nsXBLWindowKeyHandler treat eAccessKeyNotFound as
eKeyPress event even though other shortcut keys which are handled by JS
won't be executed.  Perhaps, we should stop using eAccessKeyNotFound but
it's too risky change for now.

Review commit: https://reviewboard.mozilla.org/r/217860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217860/

Comment 8

[Neil Deakin]

Comment on attachment 8948391 [details]
Bug 1435530 - part 1: Add automated tests to check Alt + D works as Ctrl + L when Alt is content access key's modifier

https://reviewboard.mozilla.org/r/217858/#review223904

Comment 9

[Neil Deakin]

Comment on attachment 8948392 [details]
Bug 1435530 - part 2: Make nsXBLWindowKeyHandler treat eAccessKeyNotFound as eKeyPress event

https://reviewboard.mozilla.org/r/217860/#review223912

This seems ok, but why doesn't a keypress event get sent as well?

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8948392 [details]
Bug 1435530 - part 2: Make nsXBLWindowKeyHandler treat eAccessKeyNotFound as eKeyPress event

https://reviewboard.mozilla.org/r/217860/#review223912

https://searchfox.org/mozilla-central/rev/f80722d4f3bfb722c5ec53880c4a7efb71285676/dom/events/EventStateManager.cpp#1172-1173
When ESM sends keypress event to all remote processes in the document, keypress event is stopped its propagation and marked as waiting reply from the remote process(es).

Then, PresShell doesn't dispatch such keypress event into the DOM tree:
https://searchfox.org/mozilla-central/rev/f80722d4f3bfb722c5ec53880c4a7efb71285676/layout/base/PresShell.cpp#7759,7780-7781,7793

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f89e76b2f3d
part 1: Add automated tests to check Alt + D works as Ctrl + L when Alt is content access key's modifier r=enndeakin+6102
https://hg.mozilla.org/integration/autoland/rev/48bca44fe6a6
part 2: Make nsXBLWindowKeyHandler treat eAccessKeyNotFound as eKeyPress event r=enndeakin+6102

Comment 12

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/1f89e76b2f3d
https://hg.mozilla.org/mozilla-central/rev/48bca44fe6a6

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Is this something that we should consider backporting to 59 or can this ride the 60 train?

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8948391 [details]
Bug 1435530 - part 1: Add automated tests to check Alt + D works as Ctrl + L when Alt is content access key's modifier

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of bug 1333459, but actually new regression of e10s for most victims.

[User impact if declined]:
Linux and Windows users cannot move focus to the URL bar with Alt + D when web content doesn't have focus.

[Is this code covered by automated tests?]:
Yes, this is the test.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No, this is completely covered by this test.

[List of other uplifts needed for the feature/fix]:
This test causes permanent orange without the patch for bug 1436248.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
This is just adding new test.

[String changes made/needed]:
No.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8948392 [details]
Bug 1435530 - part 2: Make nsXBLWindowKeyHandler treat eAccessKeyNotFound as eKeyPress event

Approval Request Comment
[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Just making nsXBLWindowKeyHandler which is handler of almost all shortcut keys listen internal event which is fired only when remote content doesn't have focus but keypress event is sent to the remote process but not handled.

[String changes made/needed]:
No.

Comment 16

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

Comment on attachment 8948392 [details]
Bug 1435530 - part 2: Make nsXBLWindowKeyHandler treat eAccessKeyNotFound as eKeyPress event

Keyboard navigation fix, sounds like this recently got worse as a regression. Let's take it for beta 10 along with the test fix.

Comment 17

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8948391 [details]
Bug 1435530 - part 1: Add automated tests to check Alt + D works as Ctrl + L when Alt is content access key's modifier

setting approval flag for the tests as well.

Comment 18

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/544407b1ac2f
https://hg.mozilla.org/releases/mozilla-beta/rev/30d165841f3d

Comment 19

[Cornel Ionce [:noni] [Hubs QA]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #14)
> [Is this code covered by automated tests?]:
> Yes, this is the test.
> [Needs manual test from QE? If yes, steps to reproduce]: 
> No, this is completely covered by this test.

Marking this issue as qe-verify- since it is covered by automated tests.