Closed Bug 1579730 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Site Identity, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 71
Tracking Status
firefox70 --- fixed
firefox71 --- fixed

People

(Reporter: flod, Assigned: flod)

References

Details

(Whiteboard: [privacy-panel][skyline])

Attachments

(1 file)

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).

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

Sounds like we need to uplift this?

Flags: needinfo?(nhnt11)
Priority: -- → P1
Whiteboard: [privacy-panel][skyline]

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

Sounds like we need to uplift this?

Yes, we should probably do it.

Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Flags: needinfo?(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?

Flags: needinfo?(francesco.lodolo)

(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.

Flags: needinfo?(francesco.lodolo)
Assignee: nhnt11 → francesco.lodolo
Pushed by flodolo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5273d28eaf3d
Use new accesskey string for "Manage Protection Settings" (contentBlocking.manageSettings2.label) r=nhnt11

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 - EventHandlerNonNull
verifyContinueCalls@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 - EventHandlerNonNull
verifyContinueCalls@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 - EventHandlerNonNull
verifyContinueCalls@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

Flags: needinfo?(francesco.lodolo)

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

Flags: needinfo?(francesco.lodolo)
Pushed by malexandru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d35b4ae0b22
Use new accesskey string for "Manage Protection Settings" (contentBlocking.manageSettings2.label) r=nhnt11
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71

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

Status: RESOLVED → VERIFIED

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
Attachment #9092024 - Flags: approval-mozilla-beta?

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.

Attachment #9092024 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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.

Flags: needinfo?(lhenry)

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

Flags: needinfo?(lhenry) → needinfo?(apavel)
Flags: needinfo?(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.

Regressions: 1582187
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: