1279143 - color indicator missing when "File -> New Container Tab" without any windows opened

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: issue.png

When opening a container via "File -> New Container Tab" under OSX while there's no windows currently opened, the colour indicator doesn't appear above the tab. The UI in the awesomebar is correctly displayed, but the colour indicator on top of the tab is missing.

* attached a screenshot to illustrate the issue

STR:

* open m-c and ensure containers are enabled via "privacy.userContext.enabled;true"
* close all the windows currently opened
* select File -> New Container Tab -> Personal Container

You'll notice that the UI under the awesomebar is correctly being displayed, but the colour indicator at the top of the tab is missing.

Build Used:
* fx50.0a1, buildID: 20160608030219, changeset: f8ad071a6e14
* https://archive.mozilla.org/pub/firefox/nightly/2016/06/2016-06-08-03-02-19-mozilla-central/

Comment 1

[Tanvi Vyas[:tanvi]]

This one is also kind of a big deal, but at least it is probably OSX specific and can only occur when no windows were originally opened.  This could easily confuse a user about what container they are in.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: tab.patch

We have to style the tab manually all the time we set usercontextid.

Comment 3

[:Gijs (he/him)]

Comment on attachment 8761484 [details] [diff] [review]
tab.patch

Review of attachment 8761484 [details] [diff] [review]:
-----------------------------------------------------------------

Can you refactor the two other places where ContextualIdentityService.setTabStyle is currently called to call tab.setUserContextId() as well, to reduce code duplication? It may be necessary to teach this method to deal with a not-yet-present linkedBrowser.

Comment 4

[:Gijs (he/him)]

In fact, with the styling being so orthogonal to the attribute on the tab, and the <browser> elements having their own property, is it actually necessary to keep the usercontextid on the tab in addition to keeping it on the browser? It seems superfluous.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: tab.patch

We still need the usercontextid attribute in the tab, but this patch centralize the calling of setuserContextId.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80ca88ff3e6a
Styling of tabs should happen all the time the attribute usercontextid is set, r=gijs

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80ca88ff3e6a

Comment 8

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Need uplift?

Comment 9

[Kamil Jozwiak [:kjozwiak]]

I opened each container via the file menu when there's no windows opened and ensured that:

* the gfx indicator at the top of the tabs is being displayed correctly
* the gfx indicator under the awesomebar is being displayed correctly

* OSX 10.11.5 x64 VM - PASSED

Used the following build for verification:
* https://archive.mozilla.org/pub/firefox/nightly/2016/07/2016-07-06-03-02-33-mozilla-central/

Comment 10

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Comment on attachment 8761689 [details] [diff] [review]
tab.patch

Approval Request Comment
[Feature/regressing bug #]:Testpilot for containers
[User impact if declined]: UI will not function in certain situation with containers test-pilot.
[Describe test coverage new/current, TreeHerder]: Some Origin Attributes testing is covered in CAPS unit tests, more is in progress.
[Risks and why]: This involves platform changes that cannot be reprecated by an addon.  In order to do a Test Pilot experiment for Containers in September / Firefox 49 release, we need this code in release.  Without it, testing is delayed until November and getting the capabilities of the feature to users is delayed even further.
[String/UUID change made/needed]: None

Comment 11

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Paul Theriault [:pauljt] from comment #10)
> Comment on attachment 8761689 [details] [diff] [review]
> tab.patch
> 
> Approval Request Comment
> [Feature/regressing bug #]:Testpilot for containers
> [User impact if declined]: UI will not function in certain situation with
> containers test-pilot.
> [Describe test coverage new/current, TreeHerder]: Some Origin Attributes
> testing is covered in CAPS unit tests, more is in progress.
> [Risks and why]: This involves platform changes that cannot be reprecated by
> an addon.  In order to do a Test Pilot experiment for Containers in
> September / Firefox 49 release, we need this code in release.  Without it,
> testing is delayed until November and getting the capabilities of the
> feature to users is delayed even further.
> [String/UUID change made/needed]: None

Commenting further to add risk details as requested via email. This is a simple fix that will correct a known issue with the Test Pilot version of containers. This change will only affect test pilot users (not regular users who dont have the containers pref enabled).

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8761689 [details] [diff] [review]
tab.patch

Needed for the Test Pilot container/identity feature, taking it.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cf93a3667b5b

Comment 14

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following builds:
* https://archive.mozilla.org/pub/firefox/nightly/2016/07/2016-07-28-00-40-10-mozilla-aurora/
** fx49.0a2, buildId: 20160728004010, changeset: 5d074cab0660

Test Cases Used:

* went through the original STR from comment#0 without any issues
* went through the test cases from comment# without any issues
* ensured that all the UI indicators are present when going through
** File -> New Container Tab
** Right Click Context Menu -> Open Link In New Container Tab
** Hamburger Menu -> Open Container Tab
** Down Arrow (when there's a lot of tabs opened) -> New Container Tab

Comment 15

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following build:

* https://archive.mozilla.org/pub/firefox/nightly/2016/07/2016-07-31-03-02-03-mozilla-central/
** fx50.0a1, buildId: 20160731030203, changeset: e5859dfe0bcb
* https://archive.mozilla.org/pub/firefox/nightly/2016/08/2016-08-02-03-04-37-mozilla-central/
** fx51.0a1, buildId: 20160802030437, changeset: ffac2798999c

Went through the test cases from comment#0, comment#9 and comment#14 without any issues.