Containers menu items needs it's own access key.

VERIFIED FIXED in Firefox 49

Status

()

defect
P1
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: jkt, Assigned: jkt)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 verified, firefox50 verified, firefox51 verified)

Details

(Whiteboard: [userContextId][domsecurity-active][uplift50+])

Attachments

(1 attachment)

As Eric law pointed out ALT+F,C did work before to call Close Tab however the menu now doesn't work.

https://twitter.com/ericlaw/status/763441199214694400

I suggest 'B' as this is unused in this menu.
Blocks: 1267916
Assignee: nobody → jkt
Hi Tanvi, is there any issue in changing this to 'B'?

Thanks
Flags: needinfo?(tanvi)
Hi Jonathan,

We should definitely change it from C to something else.  Even B if we don't have another option.  But are any of the letters from "New Container Tab" available?  That way we can continue showing an underline on a letter.  (You can see a similar problem here https://bugzilla.mozilla.org/show_bug.cgi?id=1276412#c17)

Thanks for picking up this bug!
Flags: needinfo?(tanvi)
Priority: -- → P1
Whiteboard: [userContextId][domsecurity-active]
Comment on attachment 8781151 [details]
Bug 1295160 - Changing access key of Containers menu item to B as C was currently in use

Hey Baku,
Could you review this 1 char change please :P.

I'm changing the access key over as it removed the close tab functionality.

The upper case 'B' selects the 'b' in 'Tab' and Tanvi was ok with this change on IRC.

Thanks
Attachment #8781151 - Flags: review?(amarchesini)
Comment on attachment 8781151 [details]
Bug 1295160 - Changing access key of Containers menu item to B as C was currently in use

https://reviewboard.mozilla.org/r/71644/#review69348
Attachment #8781151 - Flags: review?(amarchesini) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f2f5aa2263c1
Changing access key of Containers menu item to B as C was currently in use r=baku
Keywords: checkin-needed
Does this need uplift?  Is it an issue in Firefox 50?
Flags: needinfo?(jkt)
Whiteboard: [userContextId][domsecurity-active] → [userContextId][domsecurity-active][uplift50?]
Yup it does.
Flags: needinfo?(jkt)
Flags: needinfo?(jkt)
Comment on attachment 8781151 [details]
Bug 1295160 - Changing access key of Containers menu item to B as C was currently in use

Approval Request Comment
[Feature/regressing bug #]:
#1191442
https://hg.mozilla.org/mozilla-central/diff/9eb07902468e/browser/locales/en-US/chrome/browser/browser.dtd#l312
[User impact if declined]: users will no longer be able to close tabs with containers enabled.
[Describe test coverage new/current, TreeHerder]:
N/A
[Risks and why]:
Closing tabs using the chord alt+f,c will no longer close tabs. 
[String/UUID change made/needed]:
Flags: needinfo?(jkt)
Attachment #8781151 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/f2f5aa2263c1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(In reply to Jonathan Kingston [:jkt] from comment #9)
> [String/UUID change made/needed]:

You'll still need the l10n=(relman-approver) flag since you're touching a dtd file, but that's OK for l10n (accesskeys are locale specific).
If containers are disabled, do we still have an access key problem in Firefox 50?
Flags: needinfo?(jkt)
Duplicate of this bug: 1285853
Comment on attachment 8781151 [details]
Bug 1295160 - Changing access key of Containers menu item to B as C was currently in use

AFAIK, about:containers and related work is not planned for Fx50, given that why are we pushing to uplift this to Aurora50. I think it's best if this change rides the 51 train.
Attachment #8781151 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
(In reply to Ritu Kothari (:ritu) from comment #14)
> Comment on attachment 8781151 [details]
> Bug 1295160 - Changing access key of Containers menu item to B as C was
> currently in use
> 
> AFAIK, about:containers and related work is not planned for Fx50, given that
> why are we pushing to uplift this to Aurora50. I think it's best if this
> change rides the 51 train.

Hi Ritu,

This isn't for about:containers, it is the access key in the File menu itself.  Although Containers won't be enabled for general release in Firefox 50, it will be for Test Pilot.  So the Close Window access key won't work for Test Pilot users in Firefox 50.

I'm not sure if this is something we could fix in the test pilot addon itself.  Jonathan, do you know?

Also, to better understand the risk of not uplifting this, we need an answer to comment 12.  Kamil, perhaps you can help with that?
Flags: needinfo?(rkothari)
Flags: needinfo?(kjozwiak)
(In reply to Tanvi Vyas - out until 8/22 [:tanvi] from comment #15)
> Also, to better understand the risk of not uplifting this, we need an answer
> to comment 12.  Kamil, perhaps you can help with that?

It doesn't seem to be an issue when containers are disabled under fx50 (m-a). However, under Ubuntu, holding ALT + F and then pressing C doesn't work. You have to press ALT + F, let go of ALT + F when the file menu appears and then press C to close the tab. I think it's just the way Ubuntu deals with shortcuts but I'm not 100% sure. Jonathan, are you seeing the same thing with your Ubuntu machine?

This doesn't affect OSX correct?

Platforms Used:
* Win 10 x64 (Personal Deskop Machine) - PASSED
* Win 10 x64 VM - PASSED
* Win 8.1 x64 VM - PASSED
* Ubuntu 14.04.5 LTS VM - PASSED

Build Used:
* fx50.0a2, buildId: 20160822004020, changeset: 6f1c9caedd87
Flags: needinfo?(kjozwiak)
Comment on attachment 8781151 [details]
Bug 1295160 - Changing access key of Containers menu item to B as C was currently in use

Thanks Tanvi for the additional info on Containers and TestPilot run which is planned for Fx50. As such the patch is straight forward and worth uplifting to 50 so we provide a good experience to Containers TestPilot users. Aurora+
Flags: needinfo?(rkothari)
Attachment #8781151 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/d560a3e332dd
Whiteboard: [userContextId][domsecurity-active][uplift50?] → [userContextId][domsecurity-active][uplift50+]
Flags: needinfo?(jkt)
This needs to get uplifted into fx49 as well. Both "New Container Tab" and "Close Tab" are using the "alt+f+c" access key under the file menu.

Platforms Used:

* Win 10 x64 VM
** fx49: FAILED
* Win 8.1 x64 VM
** fx49: FAILED
* Ubuntu 16.04.1 LTS
** fx49: FAILED

Builds Used:
* https://archive.mozilla.org/pub/firefox/candidates/49.0b7-candidates/build1/
Comment on attachment 8781151 [details]
Bug 1295160 - Changing access key of Containers menu item to B as C was currently in use

Hi Ritu,

Thanks for asking about beta.  This is in fact an issue in Firefox 49.  Requesting beta uplift.

Approval Request Comment
[Feature/regressing bug #]: bug 1191442 -
https://hg.mozilla.org/mozilla-central/diff/9eb07902468e/browser/locales/en-US/chrome/browser/browser.dtd#l312
[User impact if declined]: Users testing containers in Test Pilot will not be able to use the Close Tab access key.
[Describe test coverage new/current, TreeHerder]: We don't have access key tests.
[Risks and why]: This is a one letter code change to a line of code that is containers specific.  Users that don't have containers enabled will not be affected.
[String/UUID change made/needed]: We are changing an access key from "C" to "B".
Attachment #8781151 - Flags: approval-mozilla-beta?
Went through the following verifications for fx51 and fx50 without any issues.

Platforms Used:
===============

* Win 10 x64 VM - PASSED
** fx51: PASSED
** fx50: PASSED

* Win 8.1 x64 VM - PASSED
** fx51: PASSED
** fx50: PASSED

* Ubuntu 16.04.1 LTS - PASSED
** fx51: PASSED
** fx50: PASSED

Test Cases Used:
================

* ensured that using the arrows for navigation worked once "alt+f" has been pressed
* ensured that "alt+f+c" closed the current tab without any issues when containers are enabled
* ensured that "alt+f+c" closed the current tab without any issues when containers are disabled
* ensured "ctrl + w" closed the current tab without any issues
* ensured that "alt+f+b" opened the containers context menu without any issues
** ensured "p" opens the personal container
** ensured "w" opens the personal container
** ensured "b" opens the banking container
* ensured that "alt+f+b" doesn't work or cause any issues when containers are disabled
* ensured that "alt+f+b" doesn't work under PB/Don't Remember History

Builds Used:
============

* https://archive.mozilla.org/pub/firefox/nightly/2016/08/2016-08-26-03-02-26-mozilla-central/
* https://archive.mozilla.org/pub/firefox/nightly/2016/08/2016-08-26-00-40-01-mozilla-aurora/
Comment on attachment 8781151 [details]
Bug 1295160 - Changing access key of Containers menu item to B as C was currently in use

We want to test Container in Test Pilot in 49, verified in aurora. Let's take this for beta 8.
Attachment #8781151 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Went through the following verifications for fx49 rc3 using the test cases outlined under comment#21
* build used: https://archive.mozilla.org/pub/firefox/candidates/49.0-candidates/build3/

Platforms Used:

* Win 10 x64 VM - PASSED
* Win 8.1 x64 VM - PASSED
* Ubuntu 16.04.1 LTS - PASSED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.