Closed
Bug 1316740
Opened 8 years ago
Closed 7 years ago
Deleting all your container tabs doesn't remove the top colored border
Categories
(Core :: DOM: Security, defect, P1)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: pdehaan, Assigned: baku)
References
(Blocks 1 open bug)
Details
(Whiteboard: [userContextId][domsecurity-backlog][OA])
Attachments
(1 file, 1 obsolete file)
8.22 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
1. Go to about:preferences#containers and make sure you have a few different tab containers (with different icons/colors). 2. Open a bunch of tabs in different tab containers. 3. Delete all your tab containers. ### Actual results: All your previously open tabs that were part of tab containers (and have the lovely colored top border, still have a top border, even though the tab container has been deleted). ### Expected results: After a tab container is deleted, we redraw all tabs in that container and remove the colored border.
Comment 1•8 years ago
|
||
If you try to delete a container that has a currently opened tab, we are supposed to show an alert to the user that asks them to close the open container tabs. Is that not the case yet?
Comment 2•8 years ago
|
||
(In reply to Tanvi Vyas [:tanvi] from comment #1) > If you try to delete a container that has a currently opened tab, we are > supposed to show an alert to the user that asks them to close the open > container tabs. Is that not the case yet? It looks like this part isn't working as expected. You won't receive any prompts when removing containers that are currently opened. This applies to both the default and custom containers. When you remove a container that's still opened via about:preferences#containers, the following occurs: * the awesomebar indicators are removed * the container is removed from "File -> New Container Tab" * the container is removed from the Hamburger Menu * the container is removed from the right click context menu * the container is removed from the "down arrow -> New Container Tab" * the tab indicator at the top of each tab is not being removed * loading the websites in the tab once you've removed the container will still use the userContextId The default behaviour that was supposed to be implemented in bug#1267916 was to prompt the user whenever they're removing a container that's still being used/opened.
Updated•8 years ago
|
Priority: -- → P1
Whiteboard: [userContextId][domsecurity-backlog] → [userContextId][domsecurity-backlog][OA]
Assignee | ||
Comment 4•8 years ago
|
||
Assignee: nobody → amarchesini
Attachment #8811872 -
Flags: review?(jkt)
Assignee | ||
Updated•7 years ago
|
Attachment #8811872 -
Flags: review?(jkt) → review?(gijskruitbosch+bugs)
Comment 5•7 years ago
|
||
Comment on attachment 8811872 [details] [diff] [review] delete_container.patch Review of attachment 8811872 [details] [diff] [review]: ----------------------------------------------------------------- Close, but I might as well look at this again after the changes. ::: browser/components/contextualidentity/test/browser/browser_count_and_remove.js @@ +27,4 @@ > > openTabInUserContext(1); > + is(ContextualIdentityService.countAllContainerTabs(), 2, "2 container tabs created"); > + is(ContextualIdentityService.countContainerTabs(1), 2, "2 container tabs created"); Please update this message and the ones below to include "with id 1/2" as appropriate. ::: browser/components/preferences/in-content/containers.js @@ +42,5 @@ > + let userContextId = parseInt(button.getAttribute("value"), 10); > + > + let count = ContextualIdentityService.countContainerTabs(userContextId); > + if (count == 0) { > + ContextualIdentityService.remove(userContextId); To avoid duplication and the early return, just put the confirmEx usage in an if (count > 0) instead? If the rv of the confirmEx != 0, bail out before removing stuff. ::: browser/locales/en-US/chrome/browser/preferences/preferences.properties @@ +196,5 @@ > + > +# LOCALIZATION NOTE (removeContainerMsg): Semi-colon list of plural forms. > +# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals > +# #S is the number of container tabs > +removeContainerMsg=If you remove this Container now, #S container tab will be closed. Are you sure you want to remove this Container?;If you remove this Container now, #S container tabs will be closed. Are you sure you want to remove this Container? Not sure about capitalizing "Container" in this sentence. We don't do that with "website", "page", "tab" or other concepts. Do we really want to always capitalize this (not just in all-caps-titles) ? ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm @@ +298,5 @@ > this._forEachContainerTab(function() { ++count; }); > return count; > }, > > + closeContainerTabs(userContextId) { For both this and the counting method, it'd be more idiomatic JS to have an optional userContextId argument, and just check if that was not provided (ie falsy, as a userContextId of 0 is meaningless anyway) or if it matches the usercontextid of the tab, before counting/removing the tab. Doing that would also make this a smaller patch. :-)
Attachment #8811872 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8811872 -
Attachment is obsolete: true
Attachment #8813672 -
Flags: review?(gijskruitbosch+bugs)
Comment 7•7 years ago
|
||
Comment on attachment 8813672 [details] [diff] [review] delete_container.patch Review of attachment 8813672 [details] [diff] [review]: ----------------------------------------------------------------- Last change below, with that r=me ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm @@ +287,4 @@ > let count = 0; > + this._forEachContainerTab(function(tab) { > + if (!userContextId || > + parseInt(tab.getAttribute("usercontextid"), 10) == userContextId) { And now that I'm looking at this patch, you could push the extra param down into forEachContainerTab (the two methods we're changing here are the only callers, I think), and keep the callback functions the same. :-)
Attachment #8813672 -
Flags: review?(gijskruitbosch+bugs) → review+
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5032b5290d7c Add warning messages when containers are deleted, r=Gijs
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/84db192b60ed Fix an apostrophes in preferences.properties, r=me
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5032b5290d7c https://hg.mozilla.org/mozilla-central/rev/84db192b60ed
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 11•7 years ago
|
||
This landed in 53 and Custom Containers landed in 52. This means that you can't properly delete a Container in 52. So if they are doing something "private" and expect for it to be completely deleted, there will still be remnants (existing tabs, cookies/data set by existing tabs in the contextid). This should be okay, since the UI for Custom Containers will change once we get to release, and we recommend users us Nightly if they want to experiment with Containers. Just wanted to note it here.
You need to log in
before you can comment on or make changes to this bug.
Description
•