1579730 - Old accesskey reused for new label (contentBlocking.manageSettings.accesskey)

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Francesco Lodolo [:flod]]

Bug 1572528 introduced a new label contentBlocking.manageSettings2.label but reused an existing accesskey.

That's bad for two reasons:

• The new label might not have that letter available. Since the accesskey was already translated, there's no easy way to spot an issue. When introducing a new label there should be a new accesskey, it doesn't matter if it's the same for English.
• We use a repository that covers all shipping versions of Firefox (cross-channel). That means that the accesskey can't be easily fixed (you'd need to pick a letter available in both).

Comment 1

[Johann Hofmann [:johannh]]

Feels like a P1. Nihanth, do you want to take this?

Sounds like we need to uplift this?

Comment 2

[Francesco Lodolo [:flod]]

(In reply to Johann Hofmann [:johannh] from comment #1)

Sounds like we need to uplift this?

Yes, we should probably do it.

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

So, is it ok to create a new accesskey and uplift it? And in that case, do we still need to pick a letter available in both?

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

See comment 3

Comment 5

[Francesco Lodolo [:flod]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #3)

So, is it ok to create a new accesskey and uplift it? And in that case, do we still need to pick a letter available in both?

Yes, create a new accesskey (contentBlocking.manageSettings2.label).
No, it just needs to be a character available in the associated label, not both.

Comment 6

[Francesco Lodolo [:flod]]

Attachment: Bug 1579730 - Use new accesskey string for "Manage Protection Settings" (contentBlocking.manageSettings2.label)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5273d28eaf3d
Use new accesskey string for "Manage Protection Settings" (contentBlocking.manageSettings2.label) r=nhnt11

Comment 8

[Alexandru Michis [:malexandru]]

Backed out changeset 5273d28eaf3d for failures in idbcursor-continuePrimaryKey.htm

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&tochange=c451c79196626075e659f34c4529f06bb9a3bb0b&fromchange=5273d28eaf3d0f2c5589b2811a38ce3ef07e8c15&searchStr=wpt&group_state=expanded&selectedJob=266092823

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=266092823&repo=autoland&lineNumber=14042

[task 2019-09-11T11:01:24.811Z] 11:01:24 INFO - PID 5132 | ++DOMWINDOW == 4 (000001FEEB890000) [pid = 10624] [serial = 4] [outer = 000001FEEB812020]
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO -
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - TEST-UNEXPECTED-FAIL | /IndexedDB/idbcursor-continuePrimaryKey.htm | IndexedDB: IDBCursor method continuePrimaryKey() - cursor is null
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - verifyContinueCalls/request.onsuccess<@http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:113:15
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1635:35
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - EventHandlerNonNullverifyContinueCalls@http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:109:31
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1635:35
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - EventHandlerNonNullverifyContinueCalls@http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:103:28
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1635:35
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - EventHandlerNonNullverifyContinueCalls@http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:103:28
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1635:35
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - EventHandlerNonNullverifyContinueCalls@http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:103:28
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1635:35
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - EventHandlerNonNullverifyContinueCalls@http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:103:28
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1635:35
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - EventHandlerNonNullverifyContinueCalls@http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:103:28
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - @http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:132:7
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - indexeddb_test/</open.onsuccess<@http://web-platform.test:8000/IndexedDB/support.js:131:20
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1635:35
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - EventHandlerNonNull*indexeddb_test/<@http://web-platform.test:8000/IndexedDB/support.js:128:26
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - async_test@http://web-platform.test:8000/resources/testharness.js:576:22
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - indexeddb_test@http://web-platform.test:8000/IndexedDB/support.js:105:13
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - @http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:13:15
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - TEST-OK | /IndexedDB/idbcursor-continuePrimaryKey.htm | took 511ms

Comment 9

[Francesco Lodolo [:flod]]

Per conversation on IRC, this bug is not related with the failure (that's likely bug 1168606).

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d35b4ae0b22
Use new accesskey string for "Manage Protection Settings" (contentBlocking.manageSettings2.label) r=nhnt11

Comment 11

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/7d35b4ae0b22

Comment 12

[Francesco Lodolo [:flod]]

Verified on 71.0a1 (2019-09-11) (64 bit)

Comment 13

[Francesco Lodolo [:flod]]

Comment on attachment 9092024 [details]
Bug 1579730 - Use new accesskey string for "Manage Protection Settings" (contentBlocking.manageSettings2.label)

Beta/Release Uplift Approval Request

• User impact if declined: A new label was added, reusing an existing accesskey. It might result in the accesskey using a character not available in the new text, and being appended to the label itself.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): If untranslated, string will fall back safely to en-US.
• String changes made/needed: Yes

Comment 14

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

Comment on attachment 9092024 [details]
Bug 1579730 - Use new accesskey string for "Manage Protection Settings" (contentBlocking.manageSettings2.label)

Fix for Skyline issue, verified in Nightly, let's uplift for beta 7.

Comment 15

[Andreea Pavel [:apavel]]

We tried to uplift this and receive the l10n approval error

************************* ERROR ***************************
remote:
remote: File used for localization (browser/locales/en-US/chrome/browser/browser.properties) altered in this changeset
remote: File used for localization (browser/locales/en-US/chrome/browser/browser.dtd) altered in this changeset
remote:
remote: This repository is string frozen. Please request explicit permission from
remote: release managers to break string freeze in your bug.
remote: If you have that explicit permission, denote that by including in
remote: your commit message l10n=...
remote: *************************************************************
remote:
remote: transaction abort!

Since we didn't find that info in the bug comments (tried IRC too) we abandoned uplifting this.
Please specify the l10n approvals so next shift can uplift.
Thank you.

Comment 16

[Francesco Lodolo [:flod]]

@Andreea
I'm the one usually approving l10n uplifts to beta, you can add l10n=flod

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2395cdb35d69

Comment 18

[Andreea Pavel [:apavel]]

(In reply to Francesco Lodolo [:flod] from comment #16)

@Andreea
I'm the one usually approving l10n uplifts to beta, you can add l10n=flod

I saw that on previous uplifts but I wasn't sure. Thank you, will know for future references.