Closed
Bug 1435530
Opened 7 years ago
Closed 7 years ago
Alt+D does not always work like Ctrl+L
Categories
(Core :: DOM: UI Events & Focus Handling, defect, P2)
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)
(deleted),
text/x-review-board-request
|
enndeakin
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-review-board-request
|
enndeakin
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
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•7 years ago
|
||
regressionwindow |
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
Assignee | ||
Comment 2•7 years ago
|
||
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.
Flags: needinfo?(masayuki)
Assignee | ||
Comment 3•7 years ago
|
||
(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 | ||
Updated•7 years ago
|
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Comment 5•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
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 9•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
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
Updated•7 years ago
|
Priority: -- → P2
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1f89e76b2f3d
https://hg.mozilla.org/mozilla-central/rev/48bca44fe6a6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 13•7 years ago
|
||
Is this something that we should consider backporting to 59 or can this ride the 60 train?
status-firefox58:
--- → wontfix
status-firefox59:
--- → affected
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(masayuki)
Flags: in-testsuite+
Assignee | ||
Comment 14•7 years ago
|
||
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?
Assignee | ||
Comment 15•7 years ago
|
||
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 17•7 years ago
|
||
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+
Comment 18•7 years ago
|
||
bugherder uplift |
Comment 19•7 years ago
|
||
(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-
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•