Deleting all your container tabs doesn't remove the top colored border

RESOLVED FIXED in Firefox 53

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: pdehaan, Assigned: baku)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [userContextId][domsecurity-backlog][OA])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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

2 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?
(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

2 years ago
Priority: -- → P1
Whiteboard: [userContextId][domsecurity-backlog] → [userContextId][domsecurity-backlog][OA]
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1316747
(Assignee)

Comment 4

2 years ago
Created attachment 8811872 [details] [diff] [review]
delete_container.patch
Assignee: nobody → amarchesini
Attachment #8811872 - Flags: review?(jkt)
(Assignee)

Updated

2 years ago
Attachment #8811872 - Flags: review?(jkt) → review?(gijskruitbosch+bugs)

Comment 5

2 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

2 years ago
Created attachment 8813672 [details] [diff] [review]
delete_container.patch
Attachment #8811872 - Attachment is obsolete: true
Attachment #8813672 - Flags: review?(gijskruitbosch+bugs)

Comment 7

2 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+

Comment 8

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5032b5290d7c
Add warning messages when containers are deleted, r=Gijs

Comment 9

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84db192b60ed
Fix an apostrophes in preferences.properties, r=me

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5032b5290d7c
https://hg.mozilla.org/mozilla-central/rev/84db192b60ed
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1320376
No longer depends on: 1320376

Comment 11

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