1191455 - Add URL bar decoration signifying userContextId

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Steven Englehardt [:englehardt]]

Attachment: Containers-comparison.png

The userContextId attribute is an OriginAttribute (Bug 1179985) that represents the Container (Bug 1191418) the current tab belongs to. We want to signify which Container a tab belongs to by displaying the corresponding container name and icon on the right side of the URL bar. Mock-ups of the effect are attached. Color values for the default Containers and icons given in Bug 1181953.

Comment 1

[Steven Englehardt [:englehardt]]

Attachment: 1191455.patch

WIP Patch. Applied on top of the patches in Bug 1191442.

Comment 2

[Steven Englehardt [:englehardt]]

Bram: I swapped the order of the text and the icon in my implementation, so instead of having briefcase icon on the left of 'Work' it's now on the right. Since the icons are fixed-width (and the text isn't) I think it looks better when switching between contexts. 

I was initially worried that the address bar text would flow into the container text and make things confusing, but there is a whitespace buffer and the texts are different colors so it doesn't seem like an issue.

Does this sound reasonable to you?

Comment 3

[Bram Pitoyo [:bram]]

Yes. This sounds reasonable to me.

Comment 4

[:Paolo Amadini]

Comment on attachment 8650149 [details] [diff] [review]
1191455.patch

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

::: browser/base/content/browser.js
@@ +4008,5 @@
>    let userContextMenu = document.getElementById("menu_newUserContext");
>    userContextMenu.hidden = userContextEnabled == false;
>  }
>  
> +//Updates the User Context UI indicators if the browser is in a non-default context

JSDoc-style comment.

@@ +4015,5 @@
> +  let indicator = document.getElementById("userContext-indicator");
> +  let label = document.getElementById("userContext-label");
> +  switch (browser.getAttribute("userContextId")) {
> +    case "1":
> +      label.value = "Personal";

Label values must be stored in a ".properties" file and read using a string bundle.

@@ +4019,5 @@
> +      label.value = "Personal";
> +      label.hidden = false;
> +      label.style.color = '#00a7e0';
> +      indicator.style.listStyleImage = "url('chrome://browser/skin/userContextMenuPersonal.svg')";
> +      indicator.hidden = false;

Actually, instead of "hidden" we can just set a "usercontextid" attribute conditionally on the outer container, then add a CSS rule like:

userContext-icons:not([usercontextid]) {
  display: none;
}

For other styles you can use constructs like:

userContext-icons[usercontextid="1"] > userContext-label {
  //...
}

::: browser/base/content/browser.xul
@@ +793,5 @@
> +                <label id="userContext-label"
> +                       class="userContext-label"
> +                       hidden="true"/>
> +                <image id="userContext-indicator"
> +                       class="urlbar-icon"

I think you don't need the urlbar-icon class given the other changes.

::: browser/themes/linux/browser.css
@@ +1195,5 @@
>  
> +#user-context-indicator {
> +  list-style-image: none;
> +  -moz-image-region: rect(0, 16px, 16px, 0);
> +}

The rule is unused.

Add a new #userContext-indicator rule instead and set an explicit width and height of 16px (don't use the the urlbar-icon class as this would set an unwanted padding affecting the image size).

@@ +1198,5 @@
> +  -moz-image-region: rect(0, 16px, 16px, 0);
> +}
> +
> +#userContext-label {
> +  margin-inline-end: 0px;

You can use a value of 3px here instead.

@@ +1206,5 @@
> +  -moz-box-align: center;
> +}
> +
> +.userContext-icon {
> +  padding: 0 3px;

This looks unused.

::: browser/themes/shared/userContextMenuBanking.svg
@@ +1,4 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="16" height="16" viewBox="0 0 16 16">
> +       

Please look at how other SVGs (like tracking-protection.svg) are formatted and make it as similar as possible. This requires a license header, removal of whitespace at the end of the line, and slightly different indentation.

You may want to do this on a version resized to have width="32" height="32" viewBox="0 0 32 32".

Comment 5

[Steven Englehardt [:englehardt]]

(In reply to :Paolo Amadini from comment #4)
> @@ +4019,5 @@
> > +      label.value = "Personal";
> > +      label.hidden = false;
> > +      label.style.color = '#00a7e0';
> > +      indicator.style.listStyleImage = "url('chrome://browser/skin/userContextMenuPersonal.svg')";
> > +      indicator.hidden = false;
> 
> Actually, instead of "hidden" we can just set a "usercontextid" attribute
> conditionally on the outer container, then add a CSS rule like:
> 
> userContext-icons:not([usercontextid]) {
>   display: none;
> }
> 
> For other styles you can use constructs like:
> 
> userContext-icons[usercontextid="1"] > userContext-label {
>   //...
> }

This would eliminate the need for updateUserContextUIIndicator() right? I'm not sure this is a good route to go since we eventually want to support user-defined styles. If we control the visibility entirely in CSS we would need a new rule for each ID, rather than replacing the switch statement in updateUserContextUIIndicator() with something that sets the label, icon, and style from some data store. 

Is there a way to support this in the solution you suggest?

Comment 6

[Steven Englehardt [:englehardt]]

Attachment: 1191455.patch

Addressed your comments, modulo the comment above.

Comment 7

[:Paolo Amadini]

(In reply to Steven Englehardt [:senglehardt] from comment #5)
> This would eliminate the need for updateUserContextUIIndicator() right?

You would still need to set the "usercontextid" attribute in a place that's an ancestor of the elements you're styling (in this case, userContext-icons) and set the localized label to the correct value.

> I'm
> not sure this is a good route to go since we eventually want to support
> user-defined styles. If we control the visibility entirely in CSS we would
> need a new rule for each ID, rather than replacing the switch statement in
> updateUserContextUIIndicator() with something that sets the label, icon, and
> style from some data store. 

Supporting user-defined contexts, each with its own icon and text, is a totally different level of effort and not necessarily something we would do in a first version. In that case, the design would start by answering questions like how we identify and persist those contexts, how they interact with Sync, whether we want to pull external icons, and much more.

The code we land in mozilla-central should have the quality we expect for the exact part of the feature we're implementing at present. Future iterations may change the design, in which case we'll modify the code with a similar level of quality. In this case I don't see us changing this code very soon anyways.

In this case we'd rather have the features of our theme system, like customizability of the actual icons and colors (very useful for high contrast for example), rather than implement something that gives neither customizability nor the ability to add custom contexts.

Another factor is that by doing things in a way that is similar to the existing one, we improve maintainability as most developers would know they have to look in the CSS files to find the icon references.

Hope this clarifies the reasoning!

Comment 8

[:Paolo Amadini]

Comment on attachment 8655760 [details] [diff] [review]
1191455.patch

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

Comments from the previous review still apply.

::: browser/base/content/browser.js
@@ +4021,5 @@
> + * Updates the User Context UI indicators if the browser is in a non-default context
> + */
> +function updateUserContextUIIndicator(browser)
> +{
> +  let browserBundle = Services.strings.createBundle("chrome://browser/locale/browser.properties");

You'll need a lazy getter like this:

http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#152

(I wonder if there is one for browser.properties already... couldn't find it though.)

@@ +4024,5 @@
> +{
> +  let browserBundle = Services.strings.createBundle("chrome://browser/locale/browser.properties");
> +  let indicator = document.getElementById("userContext-indicator");
> +  let label = document.getElementById("userContext-label");
> +  switch (browser.getAttribute("usercontextid")) {

Use hasAttribute to see if the is no usercontextid, in which case just clear the attribute on the other element.

If the is a usercontextid, use the switch statement to set the label value. For unknown contexts, set the label to something including the ID value (not localized), like "Context 5". This is just for experimenting and debugging - you can add a comment to that effect.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +771,5 @@
>  e10s.accessibilityNotice.disableAndRestart.accesskey = R
>  e10s.accessibilityNotice.dontDisable.label = Don't Disable
>  e10s.accessibilityNotice.dontDisable.accesskey = D
> +
> +# LOCALIZATION NOTE (usercontext.*.label)

I think localization notes are parsed and don't support wildcards, this would have to look like:

# LOCALIZATION NOTE (usercontext.personal.label, usercontext.work.label,
#                    usercontext.shopping.label, usercontext.banking.label):

@@ +774,5 @@
> +
> +# LOCALIZATION NOTE (usercontext.*.label)
> +# These strings are for different user contexts. The set of strings in Nightly
> +# will likely not carry over to release and may be user configurable, so localization
> +# is not currently needed.

It's better to use the note to describe how to activate the feature and where to look for the strings. Use terms understandable to someone unfamiliar with the feature.

I think we should localize these four words even if the feature is disabled. We're not adding hundreds of strings...

::: browser/themes/shared/usercontext/usercontext.inc.css
@@ +16,5 @@
>    list-style-image: url("chrome://browser/skin/usercontext/shopping.svg");
>  }
> +
> +#userContext-indicator {
> +  list-style-image: none;

No need for list-style-image: none, it's the default.

Comment 9

[Steven Englehardt [:englehardt]]

Attachment: 1191455.patch

Thanks for the clear explanation Paolo, it was super helpful! I agree with your reasoning and have addressed all of the comments.

Comment 10

[Steven Englehardt [:englehardt]]

Attachment: Debugging-context.png

Shows the URL bar decoration and tab decoration (Bug 1191451) for user contexts outside the 0-4 range.

Comment 11

[:Paolo Amadini]

Comment on attachment 8657283 [details] [diff] [review]
1191455.patch

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

Looks good! Please upload a new version with the following changes, no need to ask for review again.

::: browser/base/content/browser.js
@@ +4070,5 @@
> +function updateUserContextUIIndicator(browser)
> +{
> +  let hbox = document.getElementById("userContext-icons");
> +  let label = document.getElementById("userContext-label");
> +  if (browser.hasAttribute("usercontextid")) {

nit: use early returns instead of long blocks of indented code where possible.

if (!browser.hasAttribute("usercontextid")) {
  hbox.removeAttribute("usercontextid");
  return;
}

You can also move "let label" after this block.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +774,5 @@
>  
> +# LOCALIZATION NOTE (usercontext.personal.label
> +#                    usercontext.work.label
> +#                    usercontext.shopping.label
> +#                    usercontext.banking.label):

The first three names require a comma after them.

@@ +781,5 @@
> +# the context that the user is in when interacting with the site. Different
> +# contexts will store cookies and other information from those sites in
> +# different, isolated locations. You can open a new tab in a specific context by
> +# clicking File > New Container Tab > (1 of 4 contexts). Once opened, you will
> +# see these strings on the right-hand side of the URL bar.

Mention the about:config option as well.

::: browser/themes/shared/usercontext/usercontext.inc.css
@@ +31,5 @@
> +#userContext-icons:not([usercontextid]) {
> +  display: none;
> +}
> +
> +#userContext-icons[usercontextid] {

No need for [usercontextid].

Comment 12

[Steven Englehardt [:englehardt]]

Attachment: 1191455.patch

Carrying over r+ from Paolo. Updated with changes requested in previous comment.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ba18a86e004

Comment 13

[Steven Englehardt [:englehardt]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f96a4bc74e74

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/313b18c62566

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/313b18c62566