Closed Bug 1435530 Opened 2 years ago Closed 2 years ago

Alt+D does not always work like Ctrl+L

Categories

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

56 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- wontfix
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: alias-site-soft, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(2 files)

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.
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
Blocks: 1333459
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Event Handling
Ever confirmed: true
Flags: needinfo?(masayuki)
Keywords: regression
Product: Firefox → Core
Version: 58 Branch → 56 Branch
(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...
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
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
Attachment #8948391 - Flags: review?(enndeakin) → review+
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?
Attachment #8948392 - Flags: review?(enndeakin) → review+
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
Pushed by masayuki@d-toybox.com:
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
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/1f89e76b2f3d
https://hg.mozilla.org/mozilla-central/rev/48bca44fe6a6
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1436248
Is this something that we should consider backporting to 59 or can this ride the 60 train?
Flags: needinfo?(masayuki)
Flags: in-testsuite+
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.
Flags: needinfo?(masayuki)
Attachment #8948391 - Flags: approval-mozilla-beta?
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.
Attachment #8948392 - Flags: approval-mozilla-beta?
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.
Attachment #8948392 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attachment #8948391 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.