1279140 - awesomebar displaying incorrect text UI for containers

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: issue1.png

I ran into an issue where the container text UI under the awesome bar was incorrectly displaying the wrong description. Both the tab colour and the awesome bar image were correct, but the text portion of the UI was always displaying "Shopping" as it's text. I'll try reproducing and getting some STR, but in the meantime, I created this bug so we're aware that this is happening.

Examples:

* issue1.png - personal container (blue), using the correct image, however, the text UI is displaying "Shopping"
* issue2.png - work container (orange), using the correct image, however, the text UI is displaying "Shopping"

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

[Kamil Jozwiak [:kjozwiak]]

Attachment: issue2.png

Comment 2

[Tanvi Vyas[:tanvi]]

This is a big deal.  Kamil, if you figure out STR, please provide them.  Thank you!

Comment 3

[Kamil Jozwiak [:kjozwiak]]

Managed to figure it out :)

STR:

* install/launch a fresh copy of the latest m-c
* open about:config -> privacy.userContext.enabled;true
* restart m-c
* open a new container tab via file -> new container tab -> personal
* switch to the personal container tab
* select hamburger menu -> customize
* place the "open container tab" icon on the toolbar and select "exit customization"
* click on the "open container tab" icon that you've placed on the toolbar and select a work container

You'll notice that the container text under the awesomebar will appear is "Personal" rather than "Work". I've attached a video to illustrate the issue:
* https://youtu.be/wwFdu1EAeM4

Comment 4

[Andrea Marchesini [:baku]]

Attachment: label.patch

ULElement.value doesn't exist. This patch uses setAttribute().

Comment 5

[:Gijs (he/him)]

(In reply to Andrea Marchesini (:baku) from comment #4)
> Created attachment 8761483 [details] [diff] [review]
> label.patch
> 
> XULElement.value doesn't exist.

No, but the label XBL binding has a value property: http://searchfox.org/mozilla-central/source/toolkit/content/widgets/text.xml#19-20

In other words, the existing code "should work".

> This patch uses setAttribute().

Why isn't the binding present yet in this case?

Comment 6

[:Gijs (he/him)]

Comment on attachment 8761483 [details] [diff] [review]
label.patch

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

::: browser/base/content/browser.js
@@ +4126,5 @@
>  
>    let indicator = document.getElementById("userContext-indicator");
>    indicator.style.listStyleImage = "url(" + identity.icon + ")";
>  
>    hbox.hidden = false;

I suspect moving this before the label.value assignment would fix this in a more readable way.

Comment 7

[Andrea Marchesini [:baku]]

> I suspect moving this before the label.value assignment would fix this in a
> more readable way.

It doesn't. I suspect the bug is somewhere else. but using setAttribute directly it works.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0d48deeae7b
The label of the container in awesomebar should follow the container type, r=Gijs

Comment 9

[Andrea Marchesini [:baku]]

We discussed on IRC and I'll file a followup to debug more why label.value doesn't work as it should.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b0d48deeae7b

Comment 11

[Dão Gottwald [:dao]]

Please file bugs in the product / component where the relevant code lives.

Comment 12

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

Is this important enough to uplift for test pilot? Im going to er on the side of yes, unless someone else disagrees.

Comment 13

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

Comment on attachment 8761483 [details] [diff] [review]
label.patch

Approval Request Comment
[Feature/regressing bug #]: Testpilot for containers
[User impact if declined]: UI broken for testpilot users.
[Describe test coverage new/current, TreeHerder]: Testing done manually, can be easily reproduced and observed.  
[Risks and why]: One line fix to correct text. 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 14

[Kamil Jozwiak [:kjozwiak]]

Using the latest version of nightly [1], I went through verification using the STR from comment #0 without any issues. I also went through several other combinations:

* disabled/enabled containers and ensured this issue wasn't occurring once the "Open Container Tab" icon disappeared/reappeared
* rather than using the toolbar, opened several container tabs via the Hamburger Menu
* rather than opening the initial container via file -> new container tab -> personal, used the following:
** closed all FX windows and opened containers via file -> new container tab -> personal
** opened several containers via the context menu

I also ensured that the correct context is being used via the cookie manager under about:preferences#privacy

[1] https://archive.mozilla.org/pub/firefox/nightly/2016/07/2016-07-12-03-02-34-mozilla-central/

Comment 15

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

(In reply to Paul Theriault [:pauljt] from comment #13)
> Comment on attachment 8761483 [details] [diff] [review]
> label.patch
> 
> Approval Request Comment
> [Feature/regressing bug #]: Testpilot for containers
> [User impact if declined]: UI broken for testpilot users.
> [Describe test coverage new/current, TreeHerder]: Testing done manually, can
> be easily reproduced and observed.  
> [Risks and why]: One line fix to correct text. 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 clarify the risk of this change as requested via email. This is a simple one line change to correct a text value. This will not affect regular users, only users who have enabled containers manually via a preference or the Test Pilot users.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8761483 [details] [diff] [review]
label.patch

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following buid:
* 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:

* went through the original STR from comment#3 without any issues
* went through the test cases from comment#14 without any issues