1267923 - Move the CSS rules to ContextualIdentityService

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Andrea Marchesini [:baku]]

This service will store informations about the existing containers and their settings (color, icon, name, etc).

Comment 1

[Andrea Marchesini [:baku]]

We need this service not just for Containers, but also for Presentation APIs. I want to discuss about it during the next standup meeting because I have several ideas about how this service can improve gecko in many ways.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: contextualservice.patch

Comment 3

[Andrea Marchesini [:baku]]

Mike, I also have a NI for you:
in my patch, I introduce a nsIContextualIdentityService and doing that I unify quite a lot of code.
But there are still many CSS entries in our code base. I would like to get rid of them and having these style "properties" generated from the data provided by nsIContextualIdentityService. What do you think? If I can do that, I can remove domId attribute completely.

It's not clear to me if that is totally possible because many of these stylesheets are loaded as <?xml-stylesheet ...?> from XUL code.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8747627 [details] [diff] [review]
contextualservice.patch

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

feedback+'ing now, because I want to hear more about why this needs to be an XPCOM component / service.

::: browser/components/contextualidentity/moz.build
@@ +8,5 @@
>      'test/browser/browser.ini',
>  ]
>  
> +XPIDL_SOURCES += [
> +    'nsIContextualIdentityService.idl',

I don't actually think we're supposed to add new nsI stuff. Like, I think it's supposed to be mozI instead.

::: browser/components/contextualidentity/nsIContextualIdentityService.idl
@@ +7,5 @@
> +
> +interface nsISimpleEnumerator;
> +
> +[scriptable, uuid(4d20be70-b00b-4a5f-83f9-4afd82a63954)]
> +interface nsIContextualIdentityInfo : nsISupports

Is it absolutely necessary to create a new XPCOM component / service here? Would not a JSM suit your needs here, or is there some native-level stuff that's expecting to call into this?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

(In reply to Andrea Marchesini (:baku) from comment #3)
> Mike, I also have a NI for you:
> in my patch, I introduce a nsIContextualIdentityService and doing that I
> unify quite a lot of code.
> But there are still many CSS entries in our code base. I would like to get
> rid of them and having these style "properties" generated from the data
> provided by nsIContextualIdentityService. What do you think? If I can do
> that, I can remove domId attribute completely.
> 
> It's not clear to me if that is totally possible because many of these
> stylesheets are loaded as <?xml-stylesheet ...?> from XUL code.

Can you show me some of these CSS rules you're referring to?

Comment 6

[Andrea Marchesini [:baku]]

> Is it absolutely necessary to create a new XPCOM component / service here?
> Would not a JSM suit your needs here, or is there some native-level stuff
> that's expecting to call into this?

Soon new components will start using contextual identities. Probably the first will be the Presentation API.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

(In reply to Andrea Marchesini (:baku) from comment #6)
> > Is it absolutely necessary to create a new XPCOM component / service here?
> > Would not a JSM suit your needs here, or is there some native-level stuff
> > that's expecting to call into this?
> 
> Soon new components will start using contextual identities. Probably the
> first will be the Presentation API.

I'm not familiar with the Presentation API, but can I assume that the only communication mechanism that we're left with between the two APIs is XPCOM?

Comment 8

[Andrea Marchesini [:baku]]

> I'm not familiar with the Presentation API, but can I assume that the only
> communication mechanism that we're left with between the two APIs is XPCOM?

In theory yes. But that code has not been written yet.
I'm also pushing to having Private Browsing built on top of UserContextIds.

So, if there are not technical reasons, I prefer to keep it as XPCOM service.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: contextualservice.patch

Comment 10

[Andrea Marchesini [:baku]]

So far, there are no reasons to have a XPCOM service. Let's go step by step.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8749159 [details] [diff] [review]
contextualservice.patch

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

Bunch of notes below. 3 general points:

1) I'm not clear on whether we let users define their own usercontextids. If we do, then the stringbundle usage and reliance on them having access keys and/or icons seems surprising. It's not clear to me what the transition path there is and if we're implementing that in another bug or... something. :-)
2) The hardcoded colors / icon references. We can't rely on images from skin/ being present when using them from content/ (like the module). That is, if people use a third-party complete theme, those files might not exist. Likewise, the colors might not fit in. We need to think of a solution for this. We could move the images, or we could maybe use CSS variables and expect the third-party themes to define some. Again, not sure how all this plays out wrt user-defined containers.
3) it would probably still be a good idea for mconley to look at the next iteration given that he looked at the previous ones, unless he prefers to delegate the review to me. :-)

::: browser/base/content/browser.js
@@ +9,5 @@
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/NotificationDB.jsm");
>  Cu.import("resource:///modules/RecentWindow.jsm");
> +Cu.import("resource:///modules/ContextualIdentityService.jsm");

Do we use this immediately on startup? Feels like this should be a lazy getter...

::: browser/base/content/tabbrowser.xml
@@ +90,5 @@
>        <field name="mStringBundle">
>          document.getAnonymousElementByAttribute(this, "anonid", "tbstringbundle");
>        </field>
> +      <field name="ContextualIdentityService" readonly="true">
> +        (Components.utils.import("resource:///modules/ContextualIdentityService.jsm", {})).ContextualIdentityService;

You can just rely on tabbrowser.xml living in a browser window with browser.js and therefore having access to ContextualIdentityService in the window scope (ie, please get rid of the field and use ContextualIdentityService directly, without "this." in front of it)

::: browser/base/content/utilityOverlay.js
@@ +409,5 @@
> +// Populate a menu with user-context menu items. This method should be called
> +// by onpopupshowing passing the event as first argument. addCommandAttribute
> +// param is used to set the 'command' attribute in the new menuitem elements.
> +function createUserContextMenu(event, addCommandAttribute = true) {
> +  if (event.target.hasChildNodes()) {

As I noted in the other bug, if the usercontextids are going to change, we'll need to regenerate the menu when opened, so this doesn't quite work...

@@ +416,5 @@
> +
> +  let bundle = document.getElementById("bundle_browser");
> +  if (!bundle) {
> +    return false;
> +  }

When do we hit this case?

@@ +424,5 @@
> +  ContextualIdentityService.getIdentities().forEach(identity => {
> +    let menuitem = document.createElement("menuitem");
> +    menuitem.setAttribute("usercontextid", identity.userContextId);
> +    menuitem.setAttribute("label", bundle.getString(identity.label));
> +    menuitem.setAttribute("accesskey", bundle.getString(identity.accessKey));

So, this doesn't make a lot of sense to me... if we're allowing users to define these items, the labels won't be in the stringbundle, but provided by the user, right? And there likely won't be accesskeys...

::: browser/components/contextualidentity/ContextualIdentityService.jsm
@@ +46,5 @@
> +
> +  getIdentityFromId(userContextId) {
> +    // This loop is not really needed because
> +    // ContextualIdenties[i].userContextId == i+1, but this could change in the
> +    // future.

You can replace the contents of this function with:

return this._identities.find(info => info.userContextId == userContextid);

@@ +65,3 @@
>  
> +    // Display the context IDs for values outside the pre-defined range.
> +    // Used for debugging, no localization necessary.

Under what circumstances do we hit this?

@@ +71,5 @@
> +  setTabStyle(tab) {
> +    if (!tab.hasAttribute("usercontextid")) {
> +      tab.style.backgroundImage = "";
> +      tab.style.backgroundSize = "";
> +      tab.style.backgroundRepeat = "";

Nit: tab.style.removeProperty(...); for these three.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: contextualservice.patch

Thanks for the review. About having custom containers or custom UI, I want to do it in a separate step. For now, what I want is to move all the logic in 1 single component. The editing will require changes, of course, but I don't want to support them for now.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8749229 [details] [diff] [review]
contextualservice.patch

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

Hey baku, I think this looks good. I just have one concern, and I'm basically going to partially backpedal from our conversation in IRC where I said that styling within the script was probably okay.

For the extensive styling that we're doing to, for example, the tabs, I'm a bit wary of having those styles drop into script directly. I think we're going to have to find a middle ground.

See below for the idea (having talked it over with MattN and Gijs).

::: browser/base/content/browser.js
@@ +4033,5 @@
>  {
>    let hbox = document.getElementById("userContext-icons");
>  
>    if (!browser.hasAttribute("usercontextid")) {
> +    hbox.style.display = "none";

I think you can do hbox.hidden = true instead of fiddling with the style directly.

@@ +4038,4 @@
>      return;
>    }
>  
> +  let userContextId = browser.getAttribute("usercontextid");

You can actually probably fold this into the conditional above, like:

let userContextId = browser.getAttribute("usercontextid");
if (!userContextId) {
  hbox.hidden = true;
  return;
}

@@ +4041,5 @@
> +  let userContextId = browser.getAttribute("usercontextid");
> +
> +  let identity = ContextualIdentityService.getIdentityFromId(userContextId);
> +  if (!identity) {
> +    hbox.style.display = "none";

hbox.hidden = true;

@@ +4052,5 @@
> +
> +  let indicator = document.getElementById("userContext-indicator");
> +  indicator.style.listStyleImage = "url(" + identity.icon + ")";
> +
> +  hbox.style.display = "";

hbox.hidden = false;

::: browser/components/contextualidentity/ContextualIdentityService.jsm
@@ +6,4 @@
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
>  
> +const DEFAULT_TAB_COLOR = "#909090"

I wonder if we might want to pull these hardcoded colours out and use CSS variable names instead where possible.

@@ +14,5 @@
>  XPCOMUtils.defineLazyGetter(this, "gBrowserBundle", function() {
>    return Services.strings.createBundle("chrome://browser/locale/browser.properties");
>  });
>  
> +

Nit - extra newline.

@@ +56,4 @@
>  
> +    // Display the context IDs for values outside the pre-defined range.
> +    // Used for debugging, no localization necessary.
> +    return "Context " + userContextId;

We'd really better be sure we're in a pre-release channel then. Like, maybe use AppConstants.NIGHTLY_BUILD or something. I'd hate for something like that to slip into a release.

We should probably have some kind of localized fallback.

Again, good fodder for follow-up work.

@@ +71,5 @@
> +    let identity = this.getIdentityFromId(userContextId);
> +
> +    let color = identity ? identity.color : DEFAULT_TAB_COLOR;
> +
> +    tab.style.backgroundImage = "linear-gradient(to right, transparent 20%, " + color + " 30%, " + color + " 70%, transparent 80%)";

I know we already talked about this in IRC, but looking at this code now, this in-script styling really worries me.

I've been trying to think of a better solution, and I talked it over with MattN and Gijs, and this is what we came up with.

So, what we want to do here is to make sure that this stuff is maintainable as possible, and keep all of the styles in one place if we can. That makes things much simpler for the front-end team.

How do we do this, especially if we're tending towards a world where users can create an arbitrary number of contextual identities with arbitrary colours?

What our suggestion is, basically, is to use a combination of dynamic stylesheets / rules, CSS variables and JavaScript to achieve this.

Here's what we suggest:
1) Create a separate stylesheet for browser.xul that's specifically for context identities. Put as much of the default styling in that sheet as possible.

2) Create a CSS variable on the :root where you can stash a template of the linear-gradient style that you're using here. For where the color would normally go, use something like var(--context-id-dummy) or something, since you'll need to replace it dynamically later.

3) When the first window opens and has finished painting, get a reference to your stylesheet by iterating document.styleSheets looking for the one with the right chrome:// URL. Iterate the rules of that stylesheet until you can find the :root rule that you made. Grab the template, and stash that in the JSM somewhere so you don't have to do all of this searching again next time a window opens.

4) For this first window (and each subsequent window) For each context ID that the Contextual Identity Service knows about, create a CSS variable for the color on the :root. Then dynamically insert a new rule into the stylesheet for the tab styling, using the template variable you stashed before. You'll need to change the selector of the rule to match the tab with the right context ID, and you'll also need to replace the --context-id-dummy with the colour variable you added earlier in this step.

5) If a contextual id is added, removed, or modified, we're (unfortunately) going to have to go through each window's sheets, and modify the rules to keep them up to date. I guess that's unless there's a way for each window to share a single instance of a stylesheet - but I'm not certain that's possible.

Does any of that sound problematic or concerning? I know it sounds a lot, but it'll really help us keep the style stuff in the right place while giving us the dynamic behaviour that we need.

::: browser/components/contextualidentity/moz.build
@@ +8,5 @@
>      'test/browser/browser.ini',
>  ]
>  
> +EXTRA_JS_MODULES += [
> +    'ContextualIdentityService.jsm'

Nit - add trailing comma

::: browser/components/preferences/cookies.js
@@ +6,5 @@
>  const nsICookie = Components.interfaces.nsICookie;
>  
>  Components.utils.import("resource://gre/modules/PluralForm.jsm");
>  Components.utils.import("resource://gre/modules/Services.jsm")
> +Components.utils.import("resource:///modules/ContextualIdentityService.jsm");

Probably a good idea to make this lazy if you can.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: contextualservice.patch

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8749652 [details] [diff] [review]
contextualservice.patch

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

Yes, this looks much better! Thank you, baku!

I have a few questions and suggestions below, but otherwise, this looks okay to me. You should probably do a try push though and make sure we're not regressing ts_paint or tpaint in the default case.

::: browser/base/content/browser.js
@@ +7921,5 @@
> +    }
> +  },
> +};
> +
> +UserContextStyleManager.init();

This should probably not be initialized so soon - perhaps after delayedStartup.

::: browser/components/contextualidentity/ContextualIdentityService.jsm
@@ +54,3 @@
>  
> +  needsCssRule() {
> +    return this._cssRule === false;

return !this._cssRule;

@@ +68,3 @@
>      }
> +
> +    for(let i = 0; i < this._identities.length; ++i) {

Space after for, like:

for (let i...

Also, maybe a good time to use for..of, like:

for (let identity of this._identities) {
}

@@ +71,5 @@
> +      let identity = this._identities[i];
> +      rules.push(":root { --usercontext-color-" + identity.userContextId + ": " + identity.color + " }");
> +
> +      rules.push(this._cssRule.replace(/id/g, identity.userContextId));
> +      rules.push(".tabbrowser-tab[usercontextid=\"" + identity.userContextId + "\"] { background-image: var(--usercontext-tab-" + identity.userContextId + ") }");

I don't understand this second rule - what does it do?

::: browser/components/contextualidentity/content/usercontext.css
@@ +1,1 @@
> +:root {

Probably a good idea to add some documentation about these missing variables, and how we're using them to template the tab styling.

Comment 16

[Andrea Marchesini [:baku]]

> This should probably not be initialized so soon - perhaps after
> delayedStartup.

tell me more. Do you mean that I need to add an observer for browser-delayed-startup-finished ?

> @@ +71,5 @@
> > +      let identity = this._identities[i];
> > +      rules.push(":root { --usercontext-color-" + identity.userContextId + ": " + identity.color + " }");
> > +
> > +      rules.push(this._cssRule.replace(/id/g, identity.userContextId));
> > +      rules.push(".tabbrowser-tab[usercontextid=\"" + identity.userContextId + "\"] { background-image: var(--usercontext-tab-" + identity.userContextId + ") }");
> 
> I don't understand this second rule - what does it do?

It does the magic :) We have 3 rules:

1. :root { --usercontext-color-123: #color }
2. from template replacing 'id' with the userContextId: :root {   --usercontext-tab-123: linear-gradient(to right, transparent 20%, var(--usercontext-color-123) 30%, var(--usercontext-color-123) 70%, transparent 80%);
3. tabbrowser-tab[usercontextid="123"] { background-image: var(--usercontext-tab-123) }

the third rule changes the tabbrowser-tab applying the background image.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

(In reply to Andrea Marchesini (:baku) from comment #16)
> > This should probably not be initialized so soon - perhaps after
> > delayedStartup.
> 
> tell me more. Do you mean that I need to add an observer for
> browser-delayed-startup-finished ?
> 

No, I don't think that's necessary - you can add the .init to the _delayedStartup function in browser.js.


> > @@ +71,5 @@
> > > +      let identity = this._identities[i];
> > > +      rules.push(":root { --usercontext-color-" + identity.userContextId + ": " + identity.color + " }");
> > > +
> > > +      rules.push(this._cssRule.replace(/id/g, identity.userContextId));
> > > +      rules.push(".tabbrowser-tab[usercontextid=\"" + identity.userContextId + "\"] { background-image: var(--usercontext-tab-" + identity.userContextId + ") }");
> > 
> > I don't understand this second rule - what does it do?
> 
> It does the magic :) We have 3 rules:
> 
> 1. :root { --usercontext-color-123: #color }
> 2. from template replacing 'id' with the userContextId: :root {  
> --usercontext-tab-123: linear-gradient(to right, transparent 20%,
> var(--usercontext-color-123) 30%, var(--usercontext-color-123) 70%,
> transparent 80%);
> 3. tabbrowser-tab[usercontextid="123"] { background-image:
> var(--usercontext-tab-123) }
> 
> the third rule changes the tabbrowser-tab applying the background image.

Got it! Thanks. :) Would you mind adding a comment laying that out above these rules?

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ffc0498ad7e

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff1f152d6113

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9ffc0498ad7e
https://hg.mozilla.org/mozilla-central/rev/ff1f152d6113

Comment 21

[Alfred Kayser]

Type in UserContextUI.jsm:
29    icon: "chome://browser/skin/usercontext/banking.svg",

chome --> chrome

Comment 22

[Andrea Marchesini [:baku]]

Thanks Alfred! I fixed that typo in bug 1274211.