Closed Bug 1279143 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Tabbed Browser, defect, P1)

49 Branch
x86
macOS
defect

Tracking

()

VERIFIED FIXED
Firefox 50
Tracking Status
firefox49 --- verified
firefox50 --- verified

People

(Reporter: kjozwiak, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Whiteboard: [usercontextId], [domsecurity-active][uplift49+])

Attachments

(2 files, 1 obsolete file)

Attached image 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/
Priority: -- → P1
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.
Attached patch tab.patch (obsolete) — Splinter Review
We have to style the tab manually all the time we set usercontextid.
Assignee: nobody → amarchesini
Attachment #8761484 - Flags: review?(gijskruitbosch+bugs)
Whiteboard: [userContextId] → [usercontextId], [domsecurity-active]
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.
Attachment #8761484 - Flags: review?(gijskruitbosch+bugs)
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.
Attached patch tab.patchSplinter Review
We still need the usercontextid attribute in the tab, but this patch centralize the calling of setuserContextId.
Attachment #8761484 - Attachment is obsolete: true
Attachment #8761689 - Flags: review?(gijskruitbosch+bugs)
Attachment #8761689 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80ca88ff3e6a
Styling of tabs should happen all the time the attribute usercontextid is set, r=gijs
https://hg.mozilla.org/mozilla-central/rev/80ca88ff3e6a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Component: DOM: Security → Tabbed Browser
Product: Core → Firefox
Target Milestone: mozilla50 → ---
Target Milestone: --- → Firefox 50
Need uplift?
Whiteboard: [usercontextId], [domsecurity-active] → [usercontextId], [domsecurity-active][uplift49+]
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 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
Attachment #8761689 - Flags: approval-mozilla-aurora?
(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 on attachment 8761689 [details] [diff] [review]
tab.patch

Needed for the Test Pilot container/identity feature, taking it.
Attachment #8761689 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
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
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.