Closed Bug 1279140 Opened 3 years ago Closed 3 years ago

awesomebar displaying incorrect text UI for containers

Categories

(Firefox :: General, 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

(3 files)

Attached image 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/
Attached image issue2.png
Priority: -- → P1
This is a big deal.  Kamil, if you figure out STR, please provide them.  Thank you!
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
Attached patch label.patchSplinter Review
ULElement.value doesn't exist. This patch uses setAttribute().
Attachment #8761483 - Flags: review?(gijskruitbosch+bugs)
Assignee: nobody → amarchesini
Whiteboard: [userContextId] → [usercontextId], [domsecurity-active]
(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?
Flags: needinfo?(amarchesini)
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.
Attachment #8761483 - Flags: review?(gijskruitbosch+bugs)
> 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.
Flags: needinfo?(amarchesini) → needinfo?(gijskruitbosch+bugs)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0d48deeae7b
The label of the container in awesomebar should follow the container type, r=Gijs
We discussed on IRC and I'll file a followup to debug more why label.value doesn't work as it should.
Flags: needinfo?(gijskruitbosch+bugs)
Blocks: 1279271
https://hg.mozilla.org/mozilla-central/rev/b0d48deeae7b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Please file bugs in the product / component where the relevant code lives.
Component: DOM: Security → General
Product: Core → Firefox
Target Milestone: mozilla50 → Firefox 50
Is this important enough to uplift for test pilot? Im going to er on the side of yes, unless someone else disagrees.
Whiteboard: [usercontextId], [domsecurity-active] → [usercontextId], [domsecurity-active][uplift49+]
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
Attachment #8761483 - Flags: approval-mozilla-aurora?
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/
(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 on attachment 8761483 [details] [diff] [review]
label.patch

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