Details

Reviewers

flod
dimi
issammani

Group Reviewers

credential-management-reviewers
Restricted Project

Commits

rMOZILLACENTRAL7327ed6a329b: Bug 1806351 - Focusing login filter input with Ctrl+F r=credential-management…
rMOZILLACENTRAL41a86e1784a4: Bug 1806351 - Focusing login filter input with Ctrl+F r=credential-management…
rMOZILLACENTRALfc1c7e42caf7: Bug 1806351 - Focusing login filter input with Ctrl+F r=credential-management…

Bugzilla Bug ID

1806351

Test Plan

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

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Build Status

Buildable 507512
Build 603870: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".Dec 19 2022, 5:33 AM

phab-bot removed a project: secure-revision.

Herald added a subscriber: tgiles. · View Herald TranscriptDec 19 2022, 5:33 AM

sgalich added a child revision: D164992: Bug 1806352 - Restore signon.management.page.os-auth.enabled pref to it's original value r=dimi.Dec 19 2022, 5:44 AM

Code analysis found 1 defect in the diff 660693:

1 defect found by eslint (Mozlint)

IMPORTANT: Found 1 issue (error level) that must be fixed before landing.

You can run this analysis locally with:

./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 660693.

sgalich removed a child revision: D164992: Bug 1806352 - Restore signon.management.page.os-auth.enabled pref to it's original value r=dimi.Dec 19 2022, 7:10 AM

sgalich requested review of this revision.Jan 28 2023, 7:09 PM

sgalich updated this revision to Diff 674604.

sgalich retitled this revision from WIP: Bug 1806351 - Focusing login filter input with Ctrl+F to Bug 1806351 - Focusing login filter input with Ctrl+F r=#credential-management-reviewers!.

sgalich added a reviewer: credential-management-reviewers.

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 28 2023, 7:09 PM

Herald added subscribers: cmkm, flod. · View Herald Transcript

Harbormaster completed remote builds in B501506: Diff 674604.Jan 28 2023, 7:09 PM

sgalich edited the test plan for this revision. (Show Details)Jan 28 2023, 7:11 PM

1 issue closed compared to the previous diff 660693.

If you see a problem in this automated review, please report it here.

flod requested changes to this revision.Jan 28 2023, 8:08 PM

flod added inline comments.

browser/locales/en-US/browser/aboutLogins.ftl
10	You need a new ID when making this type of changes https://mozilla-l10n.github.io/documentation/localization/making_string_changes.html The new ID should also be namespaced, e.g. `about-logins-login-filter`. `focusKey` is not standard terminology for shortcuts, they're normally called just `.key` as attributes (and uppercase) about-logins-login-filter = .placeholder = Search Logins .key = F

This revision now requires changes to proceed.Jan 28 2023, 8:08 PM

sgalich added inline comments.Jan 28 2023, 9:13 PM

browser/locales/en-US/browser/aboutLogins.ftl
10	Gotcha, thanks! Is it supposed to be uppercase? I see lowercase here https://searchfox.org/mozilla-central/rev/4d2b1f753871ce514f9dccfc5b1b5e867f499229/browser/locales/en-US/browser/preferences/preferences.ftl#66 or here https://searchfox.org/mozilla-central/rev/4d2b1f753871ce514f9dccfc5b1b5e867f499229/browser/locales/en-US/browser/places.ftl#238

sgalich requested review of this revision.Jan 28 2023, 9:19 PM

sgalich updated this revision to Diff 674620.

Herald added a subscriber: MattN. · View Herald TranscriptJan 28 2023, 9:19 PM

Harbormaster completed remote builds in B501511: Diff 674620.Jan 28 2023, 9:19 PM

sgalich added inline comments.Jan 28 2023, 9:20 PM

browser/locales/en-US/browser/aboutLogins.ftl
10	It doesn't work with uppercase, because that key is used to match to the `event.key` on `keydown` event. I'll keep it lowercase.

sgalich added inline comments.Jan 28 2023, 9:24 PM

browser/locales/en-US/browser/aboutLogins.ftl
3–4	@flod does it make sense to do a follow up patch to rename all legacy names in this file to have `about-logins-` prefix? Or it will generate unnecessary work elsewhere?

sgalich edited the test plan for this revision. (Show Details)Jan 28 2023, 9:27 PM

flod added inline comments.Jan 29 2023, 5:44 AM

browser/locales/en-US/browser/aboutLogins.ftl
3–4	It will require ~100 locales to retranslate everything. To avoid that you need a migration, which will still duplicate all the strings for new locales until we drop support for ESR 102. So, no, we shouldn't do that, and just fix strings as we go.
10	That seems weird, we use uppercase most of the time https://searchfox.org/mozilla-central/search?q=.key&path=.ftl&case=false&regexp=false

flod added a subscriber: Gijs.Jan 29 2023, 6:02 AM

flod added inline comments.

browser/locales/en-US/browser/aboutLogins.ftl
10	@Gijs Do you have any clue about uppercase vs lowercase? I see most of them used in `<keyset>`. What concerns me is not much this patch, but more the behavior for users on localized builds if the key ends up uppercase.

Gijs added inline comments.Jan 29 2023, 9:15 PM

browser/locales/en-US/browser/aboutLogins.ftl
10	I believe the answer is https://searchfox.org/mozilla-central/rev/fb1e6d6e5735dcf12d96fde70351aca305961b53/dom/events/KeyEventHandler.cpp#473 - the XUL `key` event handling code lowercases both incoming events and the access key to normalize, so it doesn't matter there. Because this is a manual implementation, it would need to manually do the same.

Thanks @Gijs. We should replicate the same behavior here.

This revision now requires changes to proceed.Jan 31 2023, 1:36 PM

sgalich requested review of this revision.Feb 8 2023, 4:00 AM

sgalich updated this revision to Diff 678508.

sgalich marked 3 inline comments as done.

Harbormaster completed remote builds in B505117: Diff 678508.Feb 8 2023, 4:00 AM

Code analysis found 1 defect in the diff 678508:

1 defect found by eslint (Mozlint)

IMPORTANT: Found 1 issue (error level) that must be fixed before landing.

You can run this analysis locally with:

./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 678508.

flod accepted this revision.Feb 8 2023, 5:36 AM

sgalich updated this revision to Diff 678523.Feb 8 2023, 6:05 AM

Harbormaster completed remote builds in B505143: Diff 678523.Feb 8 2023, 6:05 AM

1 issue closed compared to the previous diff 678508.

If you see a problem in this automated review, please report it here.

dimi accepted this revision.Feb 8 2023, 3:25 PM

dimi added a project: testing-approved.

dimi added a subscriber: dimi.

dimi added inline comments.

browser/components/aboutlogins/tests/browser/browser_loginFilter.js
15	the name might be wrong?

This revision is now accepted and ready to land.Feb 8 2023, 3:25 PM

Thank you for the patch, I have a comment about mac.

browser/components/aboutlogins/content/aboutLogins.mjs
226	On mac, the default would be `command + f` and not `ctrl + f`. You can add `event.metaKey` to the check as well. This will match the command key on mac only as of Firefox 48 since the windows key is not considered meta key anymore. This will however cause both `command + f` and `ctrl + f` to work on mac, which I don't mind. The `about:addons` page excludes this behaviour by explicitly checking the platform https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/aboutaddons.js#674-680.