Closed
Bug 1295160
Opened 8 years ago
Closed 8 years ago
Containers menu items needs it's own access key.
Categories
(Core :: DOM: Security, defect, P1)
Core
DOM: Security
Tracking
()
VERIFIED
FIXED
mozilla51
People
(Reporter: jkt, Assigned: jkt)
References
Details
(Whiteboard: [userContextId][domsecurity-active][uplift50+])
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
baku
:
review+
ritu
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details |
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.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jkt
Assignee | ||
Comment 1•8 years ago
|
||
Hi Tanvi, is there any issue in changing this to 'B'?
Thanks
Flags: needinfo?(tanvi)
Comment hidden (mozreview-request) |
Comment 3•8 years ago
|
||
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]
Assignee | ||
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
mozreview-review |
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+
Assignee | ||
Updated•8 years ago
|
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
Comment 7•8 years ago
|
||
Does this need uplift? Is it an issue in Firefox 50?
Flags: needinfo?(jkt)
Whiteboard: [userContextId][domsecurity-active] → [userContextId][domsecurity-active][uplift50?]
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(jkt)
Assignee | ||
Comment 9•8 years ago
|
||
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?
Comment 10•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment 11•8 years ago
|
||
(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).
Comment 12•8 years ago
|
||
If containers are disabled, do we still have an access key problem in Firefox 50?
Flags: needinfo?(jkt)
status-firefox50:
--- → affected
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-
Comment 15•8 years ago
|
||
(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)
Comment 16•8 years ago
|
||
(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+
Comment 18•8 years ago
|
||
bugherder uplift |
Whiteboard: [userContextId][domsecurity-active][uplift50?] → [userContextId][domsecurity-active][uplift50+]
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(jkt)
Comment 19•8 years ago
|
||
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/
status-firefox49:
--- → affected
Comment 20•8 years ago
|
||
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?
Comment 21•8 years ago
|
||
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+
Comment 23•8 years ago
|
||
bugherder uplift |
Comment 24•8 years ago
|
||
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.
Description
•