Open Bug 1245262 Opened 9 years ago Updated 2 years ago

keyboard shortcuts for opening new container tabs

Categories

(Firefox :: Tabbed Browser, defect, P2)

Product:
Firefox
Component:
Tabbed Browser
Version:
47 Branch
Type:
defect

Tracking

()

Status:
REOPENED
Tracking Flags:
Tracking Status
firefox46 --- affected
firefox47 --- affected
firefox57 --- fix-optional

People

(Reporter: kjozwiak, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [userContextId][userContextId-UI][blocked])

Attachments

(2 files, 10 obsolete files)

new container tab - toolbar menu - i01
(deleted), image/png
Details
Bug 1245262 - Long press keyboard shortcut for containers menu
(deleted), text/x-review-board-request
Details
When opening a new tab via "CTRL + T" in a specific container, we should honor the contextId and open the tab using the same container rather than opening it in the default container. I'm not sure what the UX expectations would be for an average user, but from a technical perspective, I expected my new tabs to be using the same container. Example: * I'm logged into all my social media accounts & search engines that are following me via the default container * I open a new Personal container and search for something private that I don't want all the services knowing about * I quickly open a new tab from the personal container and paste something that I've copied from the previous personal container and hit "enter" quickly to start the search * After searching, I realize that I actually used the default container rather than the expected (at least in my opinion) Personal container Also, right now it's really difficult to open new containers, especially on Windows where the "Menu Bar" in fx is hidden by default. Maybe we could add another entry in the right click context menu? Thoughts?
Making CTRL+T open the same container as the one you’re currently viewing makes sense, but how can I use the same shortcut to open the default container, or any other container? My guess: I first switch to a tab with the container that I want to open next, then press CTRL+T. And if I don’t see the container I want to open, I use the toolbar menu to do it. We haven’t thought of containers access for the average user, because we want to first know whether containers is a viable solution to their problem (over something like using multiple profiles, Private Browsing, or another browser). In many ways, we don’t know whether contextual identity is a big enough problem for the average user to warrant a solution. This is why we’ve kept it hidden under a menu. However, adding a context menu that says “Open link in new [container_name] tab” is a good thing to try!
I watched the first video from our usertesting.com results today. And the user used open link in new tab. For the type of expert users that will use this feature (at least to start off with), open link in new tab is something they know how to do. So I think this bug should implement the following: * Ctrl+T will open a link with the same contextid as the current tab. * "Open Link in New [Context_Name] Tab" where [Context_Name] is replaced with the name of the current container. Then we can expand on that, in a followup bug. This second part is less important for our v1 UI. * Update the right click menu to provide options for what type of new tab you are opening. 2 options here: ** have a submenu under "Open Link in New Tab" where you can specify the container. ** have two options - one for "Open Link in New [Regular] Tab" and one for "Open Link in New [Context_Name] Tab". [Regular] could be replaced with "default" "regular" or "" (nothing at all, we just say open link in new tab). One more thing I haven't thought through fully yet - what happens when you click on the plus button? If you are looking at the last tab in a window and click the plus button, perhaps makes sense to use the same container that you are in. But if you are looking at the 2nd tab in a series of 8 tabs, and click the plus button, are you looking for the same type of tab? Or the default tab? I think we can go with the default container for v1 and add more options later.
> * Ctrl+T will open a link with the same contextid as the current tab. Ctrl+T opens a new tab. Not a link. I don't understand this proposal. About shortcut for opening containers, what about if we use this: CTRL+1,2,3,4... where the number is the container sorted by what we see in the menu.
Attached patch d.patch (obsolete) (deleted) — — Splinter Review
Paolo, this patch is about the context menu. For the rest of the bug, I'll submit different patches.
Assignee: nobody → amarchesini
Attachment #8717391 - Flags: review?(paolo.mozmail)
Comment on attachment 8717391 [details] [diff] [review] d.patch It's better to have this patch in a separate bug: 1246907
Attachment #8717391 - Flags: review?(paolo.mozmail)
Attached patch e.patch (obsolete) (deleted) — — Splinter Review
This patch is meant to be applied on top of bug 1246907.
Attachment #8717391 - Attachment is obsolete: true
Attachment #8717393 - Flags: review?(paolo.mozmail)
Comment on attachment 8717393 [details] [diff] [review] e.patch Does this work without warnings with e10s? (I guess it does if this code is executed in the content process.) If this works, maybe ask Gijs for review since he'll know if there is anything I may have missed.
Attachment #8717393 - Flags: review?(paolo.mozmail)
Comment on attachment 8717393 [details] [diff] [review] e.patch Yes, no warnings.
Attachment #8717393 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8717393 [details] [diff] [review] e.patch Review of attachment 8717393 [details] [diff] [review]: ----------------------------------------------------------------- This doesn't work in e10s, where browser.docShell is null.
Attachment #8717393 - Flags: review?(gijskruitbosch+bugs) → review-
(In reply to :Gijs Kruitbosch from comment #9) > Comment on attachment 8717393 [details] [diff] [review] > e.patch > > Review of attachment 8717393 [details] [diff] [review]: > ----------------------------------------------------------------- > > This doesn't work in e10s, where browser.docShell is null. Also, actually, this patch doesn't match comment #0 - which is about ctrl-t / new tab openings, not about opening a link in a new tab from the context menu. Wrong patch?
baku, can you clarify what this bug is about now. Is this bug to change the behavior of control+T when you are in a non-default container? Will it affect the behavior of the plus sign? We can discuss further in our Wednesday meeting as well. Thanks!
(In reply to Tanvi Vyas [:tanvi] from comment #11) > baku, can you clarify what this bug is about now. Is this bug to change the > behavior of control+T when you are in a non-default container? Will it > affect the behavior of the plus sign? We can discuss further in our > Wednesday meeting as well. Initially I wanted to implement CTRL+T opening a tab in the same container. But then, how to open a default container if you have just 1 tab that is running a non-default container? I guess that before implementing something, we must agree on what we really want here. Let's discuss it during our Wednesday meeting.
Needinfo'ing me and Bram. We have to figure out what the best thing to do here is. Kamil is also going to test ctrl+1, alt+1, window+1 on windows to see the behavior.
Flags: needinfo?(bram)
Flags: needinfo?(tanvi)
Attached image new tab icon and containers – i01 (obsolete) (deleted) —
Apologies for potentially redirecting the discussion. To me, modding the plus sign sounds more sensible than changing keyboard shortcut, because by modding (see attachment) we can add functionality without sacrificing current behaviour. Thoughts? Otherwise, if we still decide to change the Ctrl+T behaviour, you can still open the default container anytime by using the plus sign. In other words, we’re changing the behaviour of Ctrl+T from “Open a new tab”, to “Open a new tab with the same container that’s currently active in the current tab”.
Flags: needinfo?(bram)
(In reply to Bram Pitoyo [:bram] from comment #14) > Created attachment 8720644 [details] > new tab icon and containers – i01 > > Apologies for potentially redirecting the discussion. To me, modding the > plus sign sounds more sensible than changing keyboard shortcut, because by > modding (see attachment) we can add functionality without sacrificing > current behaviour. > > Thoughts? > > Otherwise, if we still decide to change the Ctrl+T behaviour, you can still > open the default container anytime by using the plus sign. > > In other words, we’re changing the behaviour of Ctrl+T from “Open a new > tab”, to “Open a new tab with the same container that’s currently active in > the current tab”. This is problematic in the overflowing tabs case because the "new tab" button becoming bigger will cause the tabs area to become smaller, leading to reflows, making it difficult to ensure the selected tab remains visible, etc. The new tab button might also have been moved elsewhere, where similarly problematic things would happen (reflowing the URL bar, for instance, or hiding/reshowing bookmarks that do/don't fit when the button is small/big). It's just a giant can of worms. Instead of on hover, why don't we create a context menu (and a submenu in the main menu) that lets you pick alternative containers *and*, whenever at least one tab for a particular container is open, we can permanently show icons for the active containers? Note that the colors used in the mock-up are a little surprising because there'll be issues with color-blindness and contrast, and we don't really use color for the normal state of any other button - so I'd probably suggest all of them should be the same shade of grey we use for other buttons on their respective OSes.
(In reply to Tanvi Vyas [:tanvi] from comment #13) > Kamil is also going to test ctrl+1, alt+1, window+1 on windows to see the > behavior. * CTRL + # --> switches between the currently opened tabs in fx * Window + # --> opens the taskbar shortcuts * ALT + # --> doesn't do anything under fx nor the OS Checked on the following Win OS's: * Win 10 x64 * Win 7 Pro x64 * Win Vista x86
Attached image new container tab - toolbar menu - i01 (deleted) —
(In reply to :Gijs Kruitbosch from comment #15) > Instead of on hover, why don't we create a context menu (and a submenu in > the main menu) that lets you pick alternative containers *and*, whenever at > least one tab for a particular container is open, we can permanently show > icons for the active containers? This is a smart idea. I’ve mocked up something similar a while back – a toolbar menu item to open containers – and it’s worth reconsidering here.
Attachment #8720644 - Attachment is obsolete: true
(In reply to Bram Pitoyo [:bram] from comment #17) > This is a smart idea. I’ve mocked up something similar a while back – a > toolbar menu item to open containers – and it’s worth reconsidering here. I think this is a great idea as well. Using the data that we got from usertesting (at least from what I've seen), most users actually suggested that the "+" button should be capable of opening container tabs. After opening 2-3 container tabs via the file menu, most of the users were pretty annoyed.
Max Popp from the UX team has mocked up a New Tab button that has an arrow beside it. Page 1–6: https://mozilla.invisionapp.com/share/6269XCTAR#/screens/139413470 This was designed for the Restore Tabs feature, but perhaps it’s a good idea to use for containers?
Clearing my needinfo here. This bug is for changes to keyboard shortcuts for new container tabs. alt+# seems the best option so far. Bug 1233886 is for changes to the plus sign for new tabs. It may be better to start with that and potentially add keyboard shortcuts along with that bug, or here as a followup.
Flags: needinfo?(tanvi)
Status: NEW → ASSIGNED
Whiteboard: [OA]
> https://mozilla.invisionapp.com/share/6269XCTAR#/screens/139413470 Is this submenu shown with right click? Because that shows another context menu where you can move the '+' button in the menu, remove it from the toolbar, and so on.
Flags: needinfo?(bram)
never mind. the conversation moved to another bug. Removing the NI.
Flags: needinfo?(bram)
Attached patch shortcut.patch (obsolete) (deleted) — — Splinter Review
We want to use CTRL+1...4.
Attachment #8717393 - Attachment is obsolete: true
Attachment #8744730 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8744730 [details] [diff] [review] shortcut.patch Review of attachment 8744730 [details] [diff] [review]: ----------------------------------------------------------------- This doesn't work - on OS X, accel maps to cmd, and cmd-0-9 are all taken (cmd-0 is reset zoom level, cmd-1-9 map to the first-eighth and last tab). Same for ctrl on Windows/Linux.
Attachment #8744730 - Flags: review?(gijskruitbosch+bugs) → review-
> This doesn't work - on OS X, accel maps to cmd, and cmd-0-9 are all taken > (cmd-0 is reset zoom level, cmd-1-9 map to the first-eighth and last tab). > Same for ctrl on Windows/Linux. I thought we checked that. Do we have a map of these commands? What about if we use ALT+1-4 on windows and macosx, and CTRL+1-9 on linux? See comment 16
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Andrea Marchesini (:baku) from comment #25) > > This doesn't work - on OS X, accel maps to cmd, and cmd-0-9 are all taken > > (cmd-0 is reset zoom level, cmd-1-9 map to the first-eighth and last tab). > > Same for ctrl on Windows/Linux. > > I thought we checked that. Do we have a map of these commands? SUMO has a decent list: https://support.mozilla.org/en-US/kb/keyboard-shortcuts-perform-firefox-tasks-quickly > What about if we use ALT+1-4 on windows and macosx, and CTRL+1-9 on linux? > See comment 16 Alt-number produces characters on OS X. Ctrl-number switches spaces, if I remember correctly. Alt on Windows normally opens/uses the menubar. Are we sure we need a shortcut at all? No recent features (Hello, Pocket, reader mode, customize mode from Australis, ...) were given a keyboard shortcut, mostly because we've run out of useful ones. It doesn't seem strictly necessary for this feature, certainly not before it's enabled by default and has proved its use. If we really wanted to do something, maybe we could customize the behaviour of accel-t, so that holding the 't' for a longer time opened the dropdown? A bit like on OS X, Chrome offers an option to only quit if you press and hold cmd-q for longer. We could open the tab immediately, but if you kept the t pressed, we could let you select the user context id?
Flags: needinfo?(gijskruitbosch+bugs)
> If we really wanted to do something, maybe we could customize the behaviour > of accel-t, so that holding the 't' for a longer time opened the dropdown? A > bit like on OS X, Chrome offers an option to only quit if you press and hold > cmd-q for longer. We could open the tab immediately, but if you kept the t > pressed, we could let you select the user context id? wow. This is interesting! I like it a lot. I want to see what Bram thinks about it.
Flags: needinfo?(bram)
Originally, I didn’t plan on assigning any keyboard shortcut for opening new container tabs. However, since container is a feature that’s meant for more advanced users, a shortcut may be necessary. We run into a couple of problems here: 1. There are already a lot of shortcuts within Firefox. What shortcut should we assign for containers, that is also somewhat related to tabs? Gijs proposed a good idea on comment 26: hold accel-t. Currently, doing this opens many new tabs. We’ll change it so that it opens a menu instead. 2. Where do we indicate this new shortcut and teach users that it exists? On the File-Edit-View menu, all item that has a shortcut is has a label on the right side. However, New Container Tab isn’t a menu item you can click. Instead, it’s an expanding submenu you hover over. Normally, expanding submenu don’t get a shortcut. 3. How do we make it clear that pressing and holding accel-t does two different things? We can take a hint from Chrome’s playbook and have some sort of a notification overlay. The way it might work is, when you press accel-t, we show this notification and say “hold accel-t to open a new container tab”. But this notification will show up every time you press accel-t, and can quickly get annoying. So I think that we should hold off on the shortcut for the moment, but keep the tab bar menu item. Thoughts?
Flags: needinfo?(bram)
(This looks more like Context Id bug than origin attributes, moving as such).
Whiteboard: [OA] → {usercontextid]
Whiteboard: {usercontextid] → [userContextId][userContextId-UI]
Marking as P1 because we need this for MVP.
Priority: -- → P1
Tanvi, I don't think we have an agreement about what this feature is. See comment 28.
Flags: needinfo?(tanvi)
(In reply to Bram Pitoyo [:bram] from comment #28) > Originally, I didn’t plan on assigning any keyboard shortcut for opening new > container tabs. > > However, since container is a feature that’s meant for more advanced users, > a shortcut may be necessary. > > We run into a couple of problems here: > > 1. There are already a lot of shortcuts within Firefox. What shortcut should > we assign for containers, that is also somewhat related to tabs? Gijs > proposed a good idea on comment 26: hold accel-t. Currently, doing this > opens many new tabs. We’ll change it so that it opens a menu instead. > Is it intentional that holding accel-t opens many tabs? I don't see how that is useful, but I could very well be overlooking the use case. I like the idea of holding the t causing the submenu to popup. It is similar to the long press idea (https://bugzilla.mozilla.org/show_bug.cgi?id=1272256). But I don't know how we communicate it. Could we implement it first and then later decide how to communicate it? If a communication is a blocker to landing, then I say we hold off on this bug until later. Other than the shortcut, is there anything else proposed in this bug that is not already covered in other bugs?
Flags: needinfo?(tanvi)
Summary: opening new tabs via "CTRL + T" should be using the same container → keyboard shortcuts for opening new container tabs
(In reply to Tanvi Vyas out 5/16-5/17 [:tanvi] from comment #32) > Is it intentional that holding accel-t opens many tabs? I don't see how > that is useful, but I could very well be overlooking the use case. Other than for browser testing purposes, I can’t think of any other reason why somebody would want to open multiple empty tabs all at once. On the other hand, we have recently designed a proper Container access point up in the tab bar, and this is a very good menu to pop open when you hold accel-t. We open this menu with the first item selected. You can hit the return key to open the first container straight away, or use the arrow keys to open a specific container on the list. It would be nice to be able to switch this behaviour on and off (under Preferences -> General -> Tabs -> “Hold accel-t to open a new Container tab”), but this complicates this bug a bit more. Perhaps we can put this behaviour under a pref? Other than that, everything else is already covered in other bugs.
(In reply to Bram Pitoyo [:bram] from comment #33) > (In reply to Tanvi Vyas out 5/16-5/17 [:tanvi] from comment #32) > > Is it intentional that holding accel-t opens many tabs? I don't see how > > that is useful, but I could very well be overlooking the use case. > > Other than for browser testing purposes, I can’t think of any other reason > why somebody would want to open multiple empty tabs all at once. > > On the other hand, we have recently designed a proper Container access point > up in the tab bar, and this is a very good menu to pop open when you hold > accel-t. We open this menu with the first item selected. You can hit the > return key to open the first container straight away, or use the arrow keys > to open a specific container on the list. > > It would be nice to be able to switch this behaviour on and off (under > Preferences -> General -> Tabs -> “Hold accel-t to open a new Container > tab”), but this complicates this bug a bit more. Perhaps we can put this > behaviour under a pref? > > Other than that, everything else is already covered in other bugs. So is the current proposal then to make hold accel-t make the down arrow menu open to "open tab in new container" section (when usercontextid is enabled). And otherwise make hold accel-t open multiple new tabs. One issue is if there are a lot of existing open tabs in the down arrow. But we can still select "open tab in new container", hit enter, and then use the submenu to choose which container. That is a lot of keys to get to a new container tab, but at least it is something that prevents the user from touching their mouse.
(In reply to Tanvi Vyas out 5/16-5/17 [:tanvi] from comment #34) > So is the current proposal then to make hold accel-t make the down arrow > menu open to "open tab in new container" section (when usercontextid is > enabled). And otherwise make hold accel-t open multiple new tabs. Yes. When usercontextid is enabled, not only will the down arrow menu be shown by default (regardless of how many tabs you currently have open), but holding accel-t will highlight the first container on this menu. > One issue is if there are a lot of existing open tabs in the down arrow. > But we can still select "open tab in new container", hit enter, and then use > the submenu to choose which container. That is a lot of keys to get to a > new container tab, but at least it is something that prevents the user from > touching their mouse. We can still make it so that, when there are many tabs open, the menu still highlights the first container on the list (but now this list is a submenu). When only a few tabs are open and user holds accel-t: http://f.cl.ly/items/081B1r412j0j0z0J223P/containers-context-menu-%20few%20tabs.png When many tabs are open and user holds accel-t: http://f.cl.ly/items/1U0X3o31210s3G320033/containers-context-menu-many%20tabs.png
Sounds good!
Blocks: 1276412
> When only a few tabs are open and user holds accel-t: > http://f.cl.ly/items/081B1r412j0j0z0J223P/containers-context-menu- > %20few%20tabs.png > > When many tabs are open and user holds accel-t: > http://f.cl.ly/items/1U0X3o31210s3G320033/containers-context-menu- > many%20tabs.png Are you working on this?
Flags: needinfo?(jkingston)
When many tabs are open and user holds accel-t: We have to decide what is this 'many' as a number. 10... 20?
Flags: needinfo?(bram)
No I'm working currently on: https://bugzilla.mozilla.org/show_bug.cgi?id=1272416 https://bugzilla.mozilla.org/show_bug.cgi?id=1274246 But as discussed this bug is blocked by that work and creating the new menu right?
Flags: needinfo?(jkingston)
Attached patch shortcut.patch (obsolete) (deleted) — — Splinter Review
Before continuing I need a feedback. The idea behind this patch is that we do it all in JS instead using XBL and XUL. An object, NewTabShortcut listens for keydown and keyup and in case those match with accel+T (or what its <key> says) it does something programmatically. After X milliseconds (1000 - to be set by UX) it should show the container menu from the down arrow button. Is this approach OK? Do you suggest something better?
Attachment #8744730 - Attachment is obsolete: true
Attachment #8758027 - Flags: feedback?(gijskruitbosch+bugs)
(In reply to Andrea Marchesini (:baku) from comment #38) > We have to decide what is this 'many' as a number. 10... 20? ‘Many’ is the point at which the down arrow menu would normally appear on the right side of the tab bar. On Mac: | Window width | Down arrow shown after opening how many tabs? | +--------------+-----------------------------------------------+ | 640 px | 5 | | 800 px | 7 | | 1024 px | 9 | | 1200 px | 12 | | 1440 px | 13 | | 1920 px | 18 |
Flags: needinfo?(bram)
Per bug https://bugzilla.mozilla.org/show_bug.cgi?id=1274246, we will show the down arrow even when theyre are not too many tabs. So any time a user with containers open holds down accel+t, it should show the container menu. As far as how long to hold it for, I would say a minimum of 1 second and maxium of 3 seconds. Bram, what do you think?
Flags: needinfo?(bram)
(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #42) > As far as how long to hold it for, I would say a minimum of 1 second and > maximum of 3 seconds. Bram, what do you think? What happens if I hold accel-t for shorter than 1 second or longer than 3 seconds? I think it’s reasonable to only open a new normal tab when you have keydown T + keyup T in a reasonably fast period of time. If you are too slow – meaning, if you keydown T for a while but don’t keyup – we detect it as open new container tab. Thoughts?
Flags: needinfo?(bram) → needinfo?(tanvi)
Attached patch shortcut.patch (obsolete) (deleted) — — Splinter Review
Attachment #8758027 - Attachment is obsolete: true
Attachment #8758027 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #8758120 - Flags: review?(gijskruitbosch+bugs)
Attached patch shortcut.patch (obsolete) (deleted) — — Splinter Review
Attachment #8758120 - Attachment is obsolete: true
Attachment #8758120 - Flags: review?(gijskruitbosch+bugs)
Attachment #8758121 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8758121 [details] [diff] [review] shortcut.patch Review of attachment 8758121 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +5796,5 @@ > + window.removeEventListener("keyup", this); > + }, > + > + handleEvent(event) { > + if (event.key != this._key || !event.accelModifier) { I tried to do a better comparison like: event.modifiers == the_key_element.getAttribute("modifiers") but this doesn't seem needed for this particular keyboard event, plus, what about if there are more modifiers? "alt shift" or "shift alt"? In theory 'modifiers' attribute contains a string of modifiers names in any order. If you prefer this approach I suggest to do: event.hasModifiers(the_key_element.getAttribute('modifiers')); so basically I implement a method that receives a string of modifiers, it parses it (reusing existing code) and then it checks if those modifiers match with what the keyboard event has.
This patch is testable only when the down arrow button is shown. Having this button always visible is task for another bug.
Comment on attachment 8758121 [details] [diff] [review] shortcut.patch Review of attachment 8758121 [details] [diff] [review]: ----------------------------------------------------------------- I don't think we need the KeyboardEvent stuff. There's a number of other issues. ::: browser/base/content/browser.js @@ +5778,5 @@ > + > + // This is for debug only... > + if (elm.getAttribute("modifiers") != 'accel') { > + dump("NewTab shortcut modifiers should be 'accel'!"); > + } Did you intend to get rid of this still? @@ +5780,5 @@ > + if (elm.getAttribute("modifiers") != 'accel') { > + dump("NewTab shortcut modifiers should be 'accel'!"); > + } > + > + this._timeout = Services.prefs.getIntPref("privacy.userContext.newTab.timeout", 1000); getIntPref on the interface has no default value, it will just throw if the pref is not defined. @@ +5782,5 @@ > + } > + > + this._timeout = Services.prefs.getIntPref("privacy.userContext.newTab.timeout", 1000); > + > + this._state = this._STATE_NONE; So as best I can tell, the 'state' corresponds 1:1 to whether or not this._timer is null or not. So let's just use that and get rid of the state mechanics. @@ +5796,5 @@ > + window.removeEventListener("keyup", this); > + }, > + > + handleEvent(event) { > + if (event.key != this._key || !event.accelModifier) { Yeah, I don't think a more generic implementation is necessary. In fact, I don't even think you need to add to KeyboardEvent. Other frontend code tends to just do something like: let accelKey = AppConstants.platform == "macosx" ? "metaKey" : "ctrlKey"; if (event.key != this._key || !event[accelKey]) { return; } @@ +5797,5 @@ > + }, > + > + handleEvent(event) { > + if (event.key != this._key || !event.accelModifier) { > + return; Shouldn't this also clear the timer etc.? @@ +5804,5 @@ > + // Let's see if this is a long press. > + if (event.type == "keydown" && this._state == this._STATE_NONE) { > + this._shiftKey = event.shiftKey; > + > + if (this._menupopup) { What are we checking for here? @@ +5822,5 @@ > + > + this._state = this._STATE_NONE; > + } > + > + event.preventDefault(); Add a comment here to say we're suppressing the default command behaviour? :-) @@ +5831,5 @@ > + this._timer = null; > + this._openContainerMenu(); > + }, > + > + _openNewTab() { Should probably not call this openNewTab if we mgiht be opening a window instead. @@ +5836,5 @@ > + BrowserOpenNewTabOrWindow(this._shiftKey); > + }, > + > + _openContainerMenu() { > + this._menupopup.showPopup(); Showing this without an anchor is... interesting. What happens in that case? @@ +7797,5 @@ > } > } > }; > > +function BrowserOpenNewTabOrWindow(shiftKeyPressed) { We likely can't change this "API": https://mxr.mozilla.org/addons/search?string=BrowserOpenNewTabOrWindow%28&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=addons Can we just make this one forward to a new one that you call from the newtabshortcut object? ::: browser/base/content/chatWindow.xul @@ +68,5 @@ > // initialise the offline listener > BrowserOffline.init(); > + > + // initialize the accel+T shortcut. > + NewTabShortcut.init(); Not sure we need this in the chat window. Do we? ::: modules/libpref/init/all.js @@ +1145,5 @@ > pref("privacy.trackingprotection.enabled", false); > // Enforce tracking protection in Private Browsing mode > pref("privacy.trackingprotection.pbmode.enabled", true); > > +pref("privacy.userContext.newTab.timeout", 1000); I expect this should live in the Firefox prefs file rather than modules/libpref/init/all.js...
Attachment #8758121 - Flags: review?(gijskruitbosch+bugs) → review-
Attached patch shortcut.patch (obsolete) (deleted) — — Splinter Review
Attachment #8758121 - Attachment is obsolete: true
Attachment #8758259 - Flags: review?(gijskruitbosch+bugs)
Attached patch shortcut.patch (obsolete) (deleted) — — Splinter Review
With pref check.
Attachment #8758259 - Attachment is obsolete: true
Attachment #8758259 - Flags: review?(gijskruitbosch+bugs)
Attachment #8758262 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8758262 [details] [diff] [review] shortcut.patch Review of attachment 8758262 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +5811,5 @@ > + return; > + } > + > + // Let's see if this is a long press. > + if (event.type == "keydown" && !this._timer) { Can we add an extra if() inside here to deal with the case where the anchor for the menu is not visible? It wouldn't do for accel-t to just be a no-op then. Something like: let allTabsButton = document.getElementById("alltabs-button"); let allTabsVisible = allTabsButton.getBoundingClientRect().width > 0 if (!allTabsVisible) { this.openNewTabOrWindow(event.shiftKey); return; } or whatever. @@ +5819,5 @@ > + } else if (event.type == "keyup") { > + // Timeout has not expired yet > + if (this._timer) { > + this._clearTimer(); > + this._openNewTabOrWindow(); Just call the non-_-prefixed version here directly? :-)
Attachment #8758262 - Flags: review?(gijskruitbosch+bugs) → review+
I need bug 1274246 before landing this patch.
Depends on: 1274246
Attached patch shortcut.patch (obsolete) (deleted) — — Splinter Review
Patch with comments applied.
Attachment #8758262 - Attachment is obsolete: true
The dependency work has been done in Bug 1272416 however both are linked.
Depends on: 1272416
(In reply to Bram Pitoyo [:bram] from comment #43) > (In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #42) > > As far as how long to hold it for, I would say a minimum of 1 second and > > maximum of 3 seconds. Bram, what do you think? > > What happens if I hold accel-t for shorter than 1 second or longer than 3 > seconds? > > I think it’s reasonable to only open a new normal tab when you have keydown > T + keyup T in a reasonably fast period of time. If you are too slow – > meaning, if you keydown T for a while but don’t keyup – we detect it as open > new container tab. > > Thoughts? Sounds good. The question is how long is a reasonably fast period of time? Should we say 2 seconds (2000 ms)?
Flags: needinfo?(tanvi) → needinfo?(bram)
Actually, I’d recommend something around 250 ms, which is the default value that Windows and OS X use. Would it be possible to first implement this value out and see how it feels? It might be too fast, for all I know, but it’s probably good to go with a system default.
Flags: needinfo?(bram)
We aren't sure if bug 1272416 is going to land, which puts this bug in a weird state. When we hold down accel+t and there are too many tabs open, we could open the down arrow. But if we don't have too many tabs open and the down arrow doesn't exist, then holding down accel+t wouldn't work for containers, and would just open a bunch of tabs. Until there were so many tabs that the down arrow appears, and then I'm not sure what would happen. Hence, we can't proceed on this bug until a decision is made about bug 1272416.
Whiteboard: [userContextId][userContextId-UI] → [userContextId][userContextId-UI][blocked]
This may turn into a keyboard shortcut option for longpress on the plus button https://bugzilla.mozilla.org/show_bug.cgi?id=1272256.
Priority: P1 → P2
No longer blocks: 1276412
I'm not working on this.
Assignee: amarchesini → nobody
Status: ASSIGNED → NEW
Assignee: nobody → jkt
I can't seem to replicate the code that triggers the shift branch of: openNewTabOrWindow(shiftKeyPressed) In my version I just end up getting either nothing happening or it opening a previously closed tab? Should I just remove this shift checking code? Thanks!
Flags: needinfo?(amarchesini)
We discussed this on IRC.
Flags: needinfo?(amarchesini)
:baku agreed that the shift modifier code should be removed or changed to just ignore these events if shift is pressed as this is used to restore a previous tab.
Flags: needinfo?(jkt)
Hi! I recently started using contextual identities, and so far I'm loving it. The ability to open a contextual tab from my keyboard is missed, a lot, and I came up with a similar wish chalked up in this thread - the ability to invoke the "longpress plus sign" menu from the keyboard. I'd also very much like the see the context "stick", as in opening a tab with Ctrl+T to stay in the current context, as that's what I personally would expect. When I do make a conscious effort I can go Alt-F (file menu) + Enter (open new tab) to "break out" of this context, or use the longpress Ctrl+T when it lands. Meaning: it's better to have current-context as default and main-context two or one key combos away, than having to kill the down arrow key every time the user wants to remain in context.
I think the behaviour that you talk about should be controlled by a pref, if anything I would like less defaults (like link clicks on a page defaulting to no containers) however I can see how that would be something that people would prefer. Perhaps we could make it that the longpress opens the menu with the current tab selected still requiring an enter etc? Either way that can be a follow up bug I think.
Attachment #8758321 - Attachment is obsolete: true
Flags: needinfo?(jkt)
Speaking to Gijs about this work it can't land is as the events will race. Log of conversation: - it's just going to race. - I don't know if there's a good way to test that kind of functionality :s - events have timestamps, can just store the first keydown timestamp and then compare when you get keyup, don't need a setTimeout - (and delete the keydown timestamp from the other events and/or when you get the keyup) but you should definitely not rely on it for something like this given there's 2 events you can use instead - fake the timestamp in like _keydownStart. storing a property on a DOM element - probably on gBrowser, ie |this| in a good part of the code in tabbrowser.xml - keep in mind that if you hold down a key, you're likely to get multiple keydown events, so don't overwrite it (unless the user stops holding 'accel' or starts using other modifiers, I guess) - anyway, then just set the keydown thing and use a single synthesizeKeypress which should fire both keydown and keyup that should work immediately :) Just uploading the patch changes so it doesn't get lost.
Comment on attachment 8814172 [details] Bug 1245262 - Long press keyboard shortcut for containers menu https://reviewboard.mozilla.org/r/95436/#review104542 Clearing review per comments over IRC and on this bug. We shouldn't rely on setTimeout here, which will also allow easier automated testing. :-)
Attachment #8814172 - Flags: review?(gijskruitbosch+bugs)
Why does Ctrl + T still not open in the same container as the current tab? It's not at all intuitive. Long press on "+" works okay, but no idea why right-clicking it doesn't show the same Containers menu; both are slower than a keyboard shortcut. Holding Ctrl + T down for longer and prompting for a container (default selection being the current one) would be a nice addition. Currently it just opens tabs infinitely which I doubt anyone really needs.
Mozilla's "Firefox Multi-Account Containers" extension adds `Ctrl-.` as a shortcut to open the container menu, which in turn allows selecting a container using the arrow keys, and opening a new tab with `Enter`. This covers the functionality discussed in this bug, which I think could be now closed. Also note that "long hold" keyboard shortcuts are not great in term of accessibility: - difficult to discover, or might be discovered by mistake - impossible with many assistive tecnologies, software or hardware - non-standard, surprising Maybe get feedback on the approach from your accessibility and UX people, they probably have more details.
>Mozilla's "Firefox Multi-Account Containers" ... covers the functionality discussed in this bug And is quite-heavy handed to make up for a deficiency, and totally ignores the feedback Mozilla is getting about Ctrl + T not retaining the current tab's context by default. >Also note that "long hold" keyboard shortcuts are not great in term of accessibility: >- difficult to discover, or might be discovered by mistake >- impossible with many assistive tecnologies, software or hardware >- non-standard, surprising - True about difficult to discover but people can look that up, as people who like shortcuts often will. - Discovering by mistake/surprising is hardly a negative when the current behaviour is "open infinite tabs". I firmly believe that losing context is more surprising. - Not working with assistive technologies/non-standard doesn't mean *nothing* should be added, or that keeping context with Ctrl + T shouldn't be fixed, or able to be chosen on the first/each invocation in a container tab. Adding better behaviour for the majority of your users will not prevent the current, lengthy sequence of keys form working: Alt + F, B, [underlined letter | first letter of container name | arrow keys, enter]. (Why one can't add their own shortcut key to a container name when the built-ins have them is another matter.) Also, how it is less non-standard than long-click with a mouse on the new tab button? How many other clickable elements in Firefox do something different when long-clicked? The point is the default behaviour isn't great and that it could be improved now before more people start using the Containers feature.
Is this still open or legacy? I'm assuming there's still no easy shortcut for creating new containers. Should we try to take this on?

4 years later, Containers not looking like they're going to disappear anytime soon, and Mozilla's position on this seems to be "We know it's a privacy hazard and awkward to use but we're not going to fix this."

Can someone explain to us how always opening a tab in the the default container instead the current one is better? Or how holding Ctrl/Cmd + T (or some other keyboard operation) is worse than infinite tab opens?

I just wanted to chime in and say that, as a Firefox end user, I would expect CTRL + T to open a tab in the same container that my current / active tab is opened in. It's annoying that I open a new tab, and have to manually assign it to the container I was just using.

@Trevor Yes, and additionally if "Select a container for each new tab" is activated it should ask in which container to put the tab.

Ctrl + Shift + Tab now provides another way to select the container when opening a new tab, but still requires significant keyboard interaction and does not solve the issue of Ctrl + T losing the current container. It should be a pref at the very least, hopefully exposed on about:preferences#containers.

It would be nice if any opening of the + menu had the current container pre-highlighted so a single tap of Enter would be enough to retain context.

(In reply to Trevor from comment #76)

I just wanted to chime in and say that, as a Firefox end user, I would expect CTRL + T to open a tab in the same container that my current / active tab is opened in. It's annoying that I open a new tab, and have to manually assign it to the container I was just using.

I would also love to have this functionality, because usually I just fire up a new tab with Ctrl+T without thinking too much but still from the right container. I am really missing this feature.

Personally I prefer the opposite. If I open a new tab, I want it to be from the default container.
I don't think I'd ever want the new tab to use the same container.

I assume you can use Ctrl+L, type url, Ctrl+Enter.

(In reply to Hugo Osvaldo Barrera from comment #80)

Personally I prefer the opposite. If I open a new tab, I want it to be from the default container.
I don't think I'd ever want the new tab to use the same container.

I assume you can use Ctrl+L, type url, Ctrl+Enter.

Just to clarify why I would prefer the behavior I mentioned.
I use the same pc to work on multiple unrelated projects, and I want to be able to "hide" different projects throughout the day.
When I work I usually just keep opening tabs, a lot of tabs, then reviewing them, then closing, and keeping some open.
Rotation of tabs is insane while I am working on something.
Though, one thing is constant - I usually work on the same thing (project) for some time, this means - one container.

At the vaty least I would love to see customization of Ctrl+T for setting this behavior (open in the same container).

PS: I try to use Ctrl+Shift+<1-9> keeys, but this is not Ctrl+T ... , and will never be ... .

As per a comment on the parallel issue on GitHub, currently the best solution is to install the New Container Tab addon. Its default shortcut to open a new tab in the same context is Alt + C. That can be buggy (doesn't work in all contexts, can conflict with normal menu/dialogue shortcuts), so you can make it more reliable by switching that to Alt + Shift + C under about:addons > ⚙️ > Manage Exstension Shortcuts.

The bug assignee didn't login in Bugzilla in the last 7 months.
:dveditz, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: jonathan → nobody
Flags: needinfo?(dveditz)

For good or ill, container UI has been relegated to the addon which tracks their issues in GitHub. Discussion should continue there.

As noted in comments above, the MAC addon does support short cuts for opening specific containers, and has https://github.com/mozilla/multi-account-containers/issues/462 for making Ctrl-T open in the same tab rather than the default one.

Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(dveditz)
Resolution: --- → MOVED
See Also: → https://github.com/mozilla/multi-account-containers/issues/462

It's all very well to say this is now an add-on problem but if there's no way for an add-on to resolve the issue, because the core API is deficient or restricted, then you've just baked the problem in forever. Extensions cannot override core shortcuts (understandable), but nor can a user do so explicitly. You've essentially locked us into to a poor UX and washed your hands of it.

In this specific case the developers of the addon are also Firefox developers (or have firefox developers on their team) and could make the necessary Core changes. They are more likely to act on an issue filed in their component and very unlikely to see this issue filed in a generic "Security" component.

"Tabbed Browser" is at least a group of folks working in this area. Maybe it's as simple as another preference choice like browser.tabs.insertAfterCurrent

Status: RESOLVED → REOPENED
Component: Security → Tabbed Browser
Product: Core → Firefox
Resolution: MOVED → ---
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: