We need something to manage containers. This is a meta-bug because every single feature for this page will be implemented in separate bugs.

Bram, I've started working on this. I need your help :) So far, this first patch adds "Open the containers manager" to any containers menu (File, hamburger icon, down arrow button) but not in the context menu (right click on a link). This option is separate with a separator (a line). Clicking this new entry, we open about:containers. This page is not part of this patch.

This second patch introduces an about:containers page that shows the list of containers. Nothing more. I want to see the UI from bram before implementing anything more complex. More important: I would like to introduce functionality as such as renaming, creating, etc etc..

Comment on attachment 8754428 [details] [diff] [review] patch 1 - Open about Containers in any existing menu (except context menu) Review of attachment 8754428 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +4026,5 @@ > } > > +function openAboutContainers() > +{ > + switchToTabHavingURI("about:containers", true); I'd really prefer not to add more global functions in browser.js unless absolutely necessary, especially if they do something trivial. We end up having to support them forevermore because add-ons, and it makes it even harder to compartmentalize / split up browser.js (which we should keep doing, IMO). I'm also not sure browser.js is loaded wherever browser-sets.inc is loaded (e.g. in mac's hidden window, so the menu structure we use if no browser windows are open and you've left the app open on mac). So please just stick the switchtotabhavinguri stuff directly in browser-sets.inc . ::: browser/base/content/utilityOverlay.js @@ +408,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, isContextMenu = false) { Nit: update the comment above this function. ::: browser/locales/en-US/chrome/browser/browser.properties @@ +726,5 @@ > userContextWork.accesskey = W > userContextBanking.accesskey = B > userContextShopping.accesskey = S > > +userContext.aboutPage.label = Open the containers manager Did this phrasing get created by UX? Feels like maybe "Manage containers" would make more sense?

Comment on attachment 8754429 [details] [diff] [review] patch 2 - about:containers Review of attachment 8754429 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/contextualidentity/content/aboutContainers.js @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +'use strict'; Nit: double quotes only in new browser code, here and below. @@ +14,5 @@ > + > +const bundle = Services.strings.createBundle('chrome://browser/locale/browser.properties'); > + > +function refreshUI() { > + let parent = document.getElementById('containers'); Nit: name the variable something more meaningful than "parent" @@ +16,5 @@ > + > +function refreshUI() { > + let parent = document.getElementById('containers'); > + while (parent.firstChild) { > + parent.removeChild(parent.firstChild); parent.firstChild.remove(); Also probably more efficient to do this with lastChild instead. @@ +40,5 @@ > + div.appendChild(sep); > +} > + > +window.addEventListener('DOMContentLoaded', function load() { > + window.removeEventListener('DOMContentLoaded', load); Doesn't seem necessary to remove this. ::: browser/components/contextualidentity/content/aboutContainers.xhtml @@ +16,5 @@ > + <title>&aboutContainers.title;</title> > + <link rel="icon" type="image/png" id="favicon" href="chrome://branding/content/icon32.png"/> > + <link rel="stylesheet" href="chrome://global/skin/in-content/common.css" type="text/css"/> > + <link rel="stylesheet" href="chrome://browser/skin/aboutContainers.css" type="text/css" /> > + <script type="application/javascript;version=1.7" src="chrome://browser/content/aboutContainers.js" /> I don't think we still need the version declaration for JS files, but I could be wrong. Just don't set a type attribute at all and check if it works. @@ +20,5 @@ > + <script type="application/javascript;version=1.7" src="chrome://browser/content/aboutContainers.js" /> > + </head> > + <body id="body"> > + <h1>&aboutContainers.title;</h1> > + <div class="page-subtitle">&aboutContainers.subtitle;</div> A subtitle should be an <h2>, but really this seems like a description which should probably just be a <p> @@ +22,5 @@ > + <body id="body"> > + <h1>&aboutContainers.title;</h1> > + <div class="page-subtitle">&aboutContainers.subtitle;</div> > + > + <div id="containers" class="tab"></div> I don't understand why this has class="tab". It will also contain a list of items so it should probably be a <ul> or <ol>. ::: browser/locales/en-US/chrome/browser/aboutContainers.dtd @@ +2,5 @@ > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + > +<!ENTITY aboutContainers.title "About Containers"> > +<!ENTITY aboutContainers.subtitle "This page helps you to manager your containers"> Typo ::: browser/themes/shared/aboutContainers.css @@ +19,5 @@ > + margin-inline-start: 0; > + margin-inline-end: 8px; > +} > + > +table { There isn't a table in the file, so this looks like copy-pasta soup from somewhere else. ::: docshell/base/nsAboutRedirector.cpp @@ +118,5 @@ > }, > #endif > + { > + "containers", "chrome://browser/content/aboutContainers.xhtml", > + nsIAboutModule::ALLOW_SCRIPT This is the wrong place. This isn't toolkit code, so it shouldn't be registered in docshell. Use the aboutredirector in browser/. ::: docshell/build/nsDocShellModule.cpp @@ +184,5 @@ > { NS_ABOUT_MODULE_CONTRACTID_PREFIX "serviceworkers", &kNS_ABOUT_REDIRECTOR_MODULE_CID }, > #ifndef ANDROID > { NS_ABOUT_MODULE_CONTRACTID_PREFIX "profiles", &kNS_ABOUT_REDIRECTOR_MODULE_CID }, > #endif > + { NS_ABOUT_MODULE_CONTRACTID_PREFIX "containers", &kNS_ABOUT_REDIRECTOR_MODULE_CID }, And this too, shouldn't be in here. ::: toolkit/content/aboutSupport.xhtml @@ +249,5 @@ > + &aboutSupport.appBasicsContainers; > + </th> > + > + <td> > + <a href="about:containers">about:containers</a> This doesn't feel like it belongs in about:support. What's the connection with about:support ?

Comment applied.

Keep in mind that this is not going to be the final UI. This is just the first patch in order to show something.

A file was missing.

Comment on attachment 8770909 [details] [diff] [review] patch 2 - about:containers Review of attachment 8770909 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/contextualidentity/content/aboutContainers.js @@ +17,5 @@ > +function refreshUI() { > + let containersList = document.getElementById("containers"); > + while (containersList.lastChild) { > + containersList.lastChild.remove();; > + } There are never any items in here and you only call it once, so this seems pointless. @@ +32,5 @@ > + containersList.appendChild(div); > + > + let name = document.createElement("h2"); > + let nameStr = bundle.GetStringFromName(identity.label); > + name.appendChild(document.createTextNode(nameStr)); name.textContent = nameStr; is much simpler. If we're just using list items you wouldn't even need an extra element. @@ +35,5 @@ > + let nameStr = bundle.GetStringFromName(identity.label); > + name.appendChild(document.createTextNode(nameStr)); > + div.appendChild(name); > + > + let sep = document.createElement("hr"); Feels like these should just be CSS borders between list items or table rows if that's what you wanted... ::: browser/components/contextualidentity/content/aboutContainers.xhtml @@ +6,5 @@ > + > +<!DOCTYPE html [ > +<!ENTITY % htmlDTD PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "DTD/xhtml1-strict.dtd"> %htmlDTD; > +<!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd"> %globalDTD; > +<!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd"> %brandDTD; You don't use either of these dtd files as far as I can tell. @@ +10,5 @@ > +<!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd"> %brandDTD; > +<!ENTITY % containersDTD SYSTEM "chrome://browser/locale/aboutContainers.dtd"> %containersDTD; > +]> > + > +<html xmlns="http://www.w3.org/1999/xhtml"> Should this or <body> have the l10n-based dir attribute? @@ +16,5 @@ > + <title>&aboutContainers.title;</title> > + <link rel="icon" type="image/png" id="favicon" href="chrome://branding/content/icon32.png"/> > + <link rel="stylesheet" href="chrome://global/skin/in-content/common.css" type="text/css"/> > + <link rel="stylesheet" href="chrome://browser/skin/aboutContainers.css" type="text/css" /> > + <script type="application/javascript" src="chrome://browser/content/aboutContainers.js" /> Don't think you actually need the script type @@ +18,5 @@ > + <link rel="stylesheet" href="chrome://global/skin/in-content/common.css" type="text/css"/> > + <link rel="stylesheet" href="chrome://browser/skin/aboutContainers.css" type="text/css" /> > + <script type="application/javascript" src="chrome://browser/content/aboutContainers.js" /> > + </head> > + <body id="body"> No need to give the body an id @@ +20,5 @@ > + <script type="application/javascript" src="chrome://browser/content/aboutContainers.js" /> > + </head> > + <body id="body"> > + <h1>&aboutContainers.title;</h1> > + <h2 class="page-subtitle">&aboutContainers.subtitle;</h2> This seems to be more of a descriptive text, and it's not on the same level as the container names you add later, and in any case 'h2' is not appropriate for subtitles (rather than section headers) - maybe just use <p> ? @@ +22,5 @@ > + <body id="body"> > + <h1>&aboutContainers.title;</h1> > + <h2 class="page-subtitle">&aboutContainers.subtitle;</h2> > + > + <div id="containers"></div> This is a list or a table/grid, so it shouldn't just be a <div>. ::: browser/locales/en-US/chrome/browser/aboutContainers.dtd @@ +2,5 @@ > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + > +<!ENTITY aboutContainers.title "About Containers"> > +<!ENTITY aboutContainers.subtitle "This page helps you to manage your containers"> Nit: not really a subtitle, and this should have a full stop at the end. ::: browser/themes/shared/aboutContainers.css @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +html { > + --aboutContainers-table-background: #ebebeb; Unused? @@ +3,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +html { > + --aboutContainers-table-background: #ebebeb; > + background-color: var(--in-content-page-background); This is already set here: http://searchfox.org/mozilla-central/rev/bfcc10319e4e3ce78367fa9bba9316f7eb5248b6/toolkit/themes/shared/in-content/common.inc.css#48 so that seems unnecessary. @@ +7,5 @@ > + background-color: var(--in-content-page-background); > +} > + > +body { > + margin: 40px 48px; Why do we need to change anything here compared to what the in-content stylesheets already do?

Comment on attachment 8771378 [details] [diff] [review] patch 3 - UI for the main page Review of attachment 8771378 [details] [diff] [review]: ----------------------------------------------------------------- There's too much feedback on the previous patch, and this seems to be half-done (there's a 'create' button that doesn't do anything?). I don't think this can land. ::: browser/components/contextualidentity/content/aboutContainers.js @@ +31,4 @@ > let containersList = document.getElementById("containers"); > > + let divRow = document.createElement("div"); > + divRow.setAttribute("class", "row"); This is just a giveaway that this should be a table instead... ::: browser/locales/en-US/chrome/browser/aboutContainers.properties @@ +1,3 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. If we're having our own file can we move the other localized strings to this bundle as well (instead of browser.properties) ?

> > +body { > > + margin: 40px 48px; > > Why do we need to change anything here compared to what the in-content > stylesheets already do? This is needed otherwise there is no border at all.

1. I merged the 2 patches. 2. sorry for the horrible patches... CSS/HTML are not my thing (yet).

And... important point: I don't want to land these patches yet. But I want to go step by step because I don't know if what I'm doing makes sense or not :)

Comment on attachment 8771435 [details] [diff] [review] patch 2 - about:containers Review of attachment 8771435 [details] [diff] [review]: ----------------------------------------------------------------- I haven't looked at the CSS in detail because the HTML structure is still changing so the CSS will change, too. ::: browser/components/contextualidentity/content/aboutContainers.js @@ +11,5 @@ > + > +XPCOMUtils.defineLazyModuleGetter(this, "ContextualIdentityService", > + "resource://gre/modules/ContextualIdentityService.jsm"); > + > +const bundle = Services.strings.createBundle("chrome://browser/locale/browser.properties"); As I noted earlier, it would be nice to move these strings out of browser.properties if possible. If not, please name the variable "browserBundle" instead. @@ +14,5 @@ > + > +const bundle = Services.strings.createBundle("chrome://browser/locale/browser.properties"); > +const containersBundle = Services.strings.createBundle('chrome://browser/locale/aboutContainers.properties'); > + > +function display(identity, lastIdentity) { Why do we need the lastIdentity thing? @@ +17,5 @@ > + > +function display(identity, lastIdentity) { > + let containersList = document.getElementById("containers"); > + > + let divRow = document.createElement("div"); This looks like it still wants to be a table, so let's make it a table? :-) @@ +39,5 @@ > + divRow.appendChild(divControls); > + > + let preferencesButton = document.createElement("button"); > + preferencesButton.setAttribute("class", "container-button"); > + preferencesButton.appendChild(document.createTextNode(containersBundle.GetStringFromName("aboutContainers.preferencesButton"))); so instead of appending textNodes, use: preferencesButton.textNode = "whatever"; and similarly for the removeButton below. @@ +41,5 @@ > + let preferencesButton = document.createElement("button"); > + preferencesButton.setAttribute("class", "container-button"); > + preferencesButton.appendChild(document.createTextNode(containersBundle.GetStringFromName("aboutContainers.preferencesButton"))); > + divControls.appendChild(preferencesButton); > + preferencesButton.onclick = function() { preferences(identity.userContextId); } What would the prefs for an identity be? @@ +47,5 @@ > + let removeButton = document.createElement("button"); > + removeButton.setAttribute("class", "container-button"); > + removeButton.appendChild(document.createTextNode(containersBundle.GetStringFromName("aboutContainers.removeButton"))); > + divControls.appendChild(removeButton); > + removeButton.onclick = function() { remove(identity.userContextId); } It would probably make sense to have just one click listener on the table ('delegate' style event listeners) and check from there which button in which row got clicked. You can store the identity id as an attribute on the row with something like setAttribute("data-identity", identity) and then read with element.dataset.identity, IIRC.

> As I noted earlier, it would be nice to move these strings out of > browser.properties if possible. If not, please name the variable > "browserBundle" instead. I prefer to implement an containers.properties, but in a separate patch. > This looks like it still wants to be a table, so let's make it a table? :-) Any reason why a table is better than what I wrote? > What would the prefs for an identity be? Change the title, the icon and the color.

(In reply to Andrea Marchesini (:baku) from comment #15) > > This looks like it still wants to be a table, so let's make it a table? :-) > > Any reason why a table is better than what I wrote? Well, semantically, what is this? It should either be a list (so <ul> with <li> items) or a table (so <table> with each <tr> row representing one identity, and one cell for the icon, color, label and buttons). <div> has no semantic meaning, and so it's not really the right thing to use for a list of items.

Comment on attachment 8771796 [details] [diff] [review] patch 2 - about:containers Review of attachment 8771796 [details] [diff] [review]: ----------------------------------------------------------------- Getting there, next set of feedback below. ::: browser/components/contextualidentity/content/aboutContainers.js @@ +18,5 @@ > +function display(identity) { > + let containersList = document.getElementById("containers"); > + > + let tr = document.createElement("tr"); > + tr.setAttribute("class", "row"); You don't need to set a class. :-) @@ +19,5 @@ > + let containersList = document.getElementById("containers"); > + > + let tr = document.createElement("tr"); > + tr.setAttribute("class", "row"); > + containersList.appendChild(tr); Typically you append the containing element of what you're appending to the DOM last, so that everything is there at the same time, reducing the number of layout changes. @@ +34,5 @@ > + tdDesc.setAttribute("class", "description"); > + tr.appendChild(tdDesc); > + > + let name = document.createElement("h3"); > + name.textContent = bundle.GetStringFromName(identity.label); Instead of nesting, you could assign the text into the <td> directly and give it a class, which you can then use to style the text to be the appropriate size (if you feel it needs to be bigger or bolder or whatever <h3> is doing). It's not really a header, so <h3> isn't quite right. In fact, it should probably be a <th> and have the scope=row attribute. Then you also don't need a class, and can just select on 'th' to give it the style you want. @@ +47,5 @@ > + preferencesButton.setAttribute("data-op", "preferences"); > + preferencesButton.textContent = containersBundle.GetStringFromName("aboutContainers.preferencesButton"); > + tdPreferences.appendChild(preferencesButton); > + > + let tdRemove = document.createElement("td"); I expect you could probably just use a single <td> for the buttons - not sure there's a point in using a separate one for each button. @@ +52,5 @@ > + tr.appendChild(tdRemove); > + > + let removeButton = document.createElement("button"); > + removeButton.setAttribute("class", "container-button"); > + removeButton.setAttribute("data-userContextId", identity.userContextId); You could store the data-userContextId on the row, which means you can store it once and access it from any of the buttons. ::: browser/components/contextualidentity/content/aboutContainers.xhtml @@ +23,5 @@ > + <h1>&aboutContainers.title;</h1> > + <a id="help-link-icon" href="#" target="_blank"></a> > + </header> > + > + <table id="containers"></table> Typically tables have a set of headers indicating what each column represents... would that make sense here? ::: browser/themes/shared/aboutContainers.css @@ +7,5 @@ > +} > + > +table { > + width: 100%; > + border-collapse: collapse; Nit: trailing whitespace @@ +10,5 @@ > + width: 100%; > + border-collapse: collapse; > +} > + > +tr.row td { Shouldn't this just have "td" as the selector? @@ +14,5 @@ > +tr.row td { > + border-bottom: 1px solid var(--in-content-border-color); > +} > + > +tr.row:last-of-type td { This doesn't need the class selector @@ +29,5 @@ > + border-bottom: 1px solid var(--in-content-border-color); > + margin-bottom: 1.5em; > +} > + > +header #help-link-icon { The #id selector doesn't need the tagname + descendant selector in front of it. But it seems you've missed an :active selector -- instead, can you update this set of selectors: http://searchfox.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#260-281 to include the identical: html|*.help-button, html|*.help-button:hover, html|*.help-button:hover:active, selectors, and use a <button> with the .help-button class? I think that will get us the right styling, then you don't need to do anything here. @@ +36,5 @@ > + min-height: 24px; > + background-image: url("chrome://global/skin/in-content/help-glyph.svg#help"); > +} > + > +header #help-link-icon:hover { Same selector issue

Comment on attachment 8772402 [details] [diff] [review] patch 2 - about:containers Review of attachment 8772402 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the below addressed, and assuming we land this together with followup patches that actually implement functionality for the buttons etc. :-) ::: browser/components/contextualidentity/content/aboutContainers.js @@ +28,5 @@ > + let icon = document.createElement("img"); > + icon.src = identity.icon; > + tdImg.appendChild(icon); > + > + let tdDesc = document.createElement("td"); As noted before, I think this should be a <th>, with the scope=row attribute. I think that will also mean you won't need font-weight: bold in the CSS anymore. @@ +42,5 @@ > + preferencesButton.setAttribute("data-op", "preferences"); > + preferencesButton.textContent = containersBundle.GetStringFromName("aboutContainers.preferencesButton"); > + tdPreferences.appendChild(preferencesButton); > + > + let tdRemove = document.createElement("td"); Was there a reason not to put both buttons in the same table cell? @@ +68,5 @@ > +}); > + > +function clickManager(event) { > + if (event.target.dataset.op == "preferences") { > + preferences(event.target.dataset.userContextId); This needs to get the userContextId of the row now. Probably event.target.closest('tr').dataset.userContextId, or something.

baku, can you add a screenshot of what the page looks like with these patches? Thanks!

Review commit: https://reviewboard.mozilla.org/r/66878/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66878/

Comment on attachment 8773360 [details] [diff] [review] WIP Used this in my patch but its not like this at all really. Thanks!

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66878/diff/1-2/

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page As you had points on this before would you be able to take another look please?

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66878/diff/2-3/

https://reviewboard.mozilla.org/r/66878/#review63966 ::: browser/components/contextualidentity/content/aboutContainers.js:159 (Diff revision 3) > + color: defaultContainerColor > + }; > + let title = containersBundle.GetStringFromName("aboutContainers.newContainerTitle"); > + if (userContextId) { > + identity = ContextualIdentityService.getIdentityFromId(parseInt(userContextId)); > + title = identity.name + " " + containersBundle.GetStringFromName("aboutContainers.updateContainerTitle"); This should receive the identity.name. Use getFormattedString.

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66878/diff/3-4/

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page https://reviewboard.mozilla.org/r/66878/#review64100 Unfortunately there are a number of issues with this patch. Can you have a look below and maybe flag e.g. :jaws or :mikedeboer for review for the next iteration while I'm away? Thanks! ::: browser/components/contextualidentity/content/aboutContainers.js:37 (Diff revision 4) > + let controls = document.createElement("div"); > + controls.className = "controls"; > + row.appendChild(controls); > > let icon = document.createElement("img"); > icon.src = identity.icon; While we're here: this should have the alt attribute set to an empty string. ::: browser/components/contextualidentity/content/aboutContainers.js:80 (Diff revision 4) > + display(identity); > -}); > + }); > +} > > function clickManager(event) { > + let userContextId = event.target.closest('li').dataset.userContextId; We should parseInt here and not rely on the remove/preferences methods to do that themselves. ::: browser/components/contextualidentity/content/aboutContainers.js:98 (Diff revision 4) > - // TODO > + ContextualIdentityService.remove(parseInt(userContextId)); > + refreshUI(); > } > > -function remove(userContextId) { > - // TODO > +function createModal(options) { > + let overlay = document.createElement("div"); Please can we just put the markup in the document and hide it, and only show it when we need to show it? Instead of the `bodyCallback` you could just use `replaceChild` to swap out bodies (and hide/unhide them). ::: browser/components/contextualidentity/content/aboutContainers.js:115 (Diff revision 4) > + let title = document.createElement("h3"); > + title.textContent = options.title; > + modalHeader.appendChild(title); > + > + let closeMethod = function () { > + overlay.parentNode.removeChild(overlay); overlay.remove(); ::: browser/components/contextualidentity/content/aboutContainers.js:121 (Diff revision 4) > + }; > + > + let close = document.createElement("button"); > + close.className = "close-icon"; > + close.setAttribute("role", "button"); > + close.setAttribute("aria-label", "Close"); This needs to be localized. ::: browser/components/contextualidentity/content/aboutContainers.js:141 (Diff revision 4) > + //Fixes very small screen sizes > + window.scroll(0, 0); Seems like the modal should just be position: fixed? Then you won't need this. ::: browser/components/contextualidentity/content/aboutContainers.js:164 (Diff revision 4) > + title = containersBundle.formatStringFromName("aboutContainers.updateContainerTitle", [identity.name], 1); > + } > + > + createModal({ > + parentElement: document.body, > + title: title, Don't need to repeat this value. ::: browser/components/contextualidentity/content/aboutContainers.js:169 (Diff revision 4) > + title: title, > + bodyCallback: (modalBody, closeMethod) => { > + let form = document.createElement("form"); > + form.className = "container-form"; > + > + form.addEventListener("submit", (event) => { This should prevent default on the event. ::: browser/components/contextualidentity/content/aboutContainers.js:173 (Diff revision 4) > + formValues.get("name") || defaultContainerName, > + formValues.get("icon") || defaultContainerIcon, > + formValues.get("color") || defaultContainerColor); We should preselect and prefill these in the UI rather than putting in the defaults here. If we don't want to do that, we should ensure that you can't accept the dialog without having made choices. It's also odd that you could update an item here while not providing some of the required information, thus reverting it to the default. Perhaps the form fields need a sprinkling of 'required' attributes? ::: browser/components/contextualidentity/content/aboutContainers.js:187 (Diff revision 4) > + refreshUI(); > + }); > + > + let nameLabel = document.createElement("label"); > + nameLabel.for = "name"; > + nameLabel.textContent = containersBundle.GetStringFromName("aboutContainers.nameLabel") + ":"; The colon should be part of the l10n string. Same in the other label entities. Other languages might use other characters. ::: browser/components/contextualidentity/content/aboutContainers.js:201 (Diff revision 4) > + iconLabel.for = "iconbutton"; > + iconLabel.textContent = containersBundle.GetStringFromName("aboutContainers.iconLabel") + ":"; > + form.appendChild(createFormRow(iconLabel, createIconButtons(identity.icon))); > + > + let colorLabel = document.createElement("label"); > + colorLabel.for = "colorswatch"; This is incorrect - there isn't an element with this ID. Instead, give the label an ID and mark the <radio> elements up as being labeled by the label element, or make the <label> contain the radio elements. ::: browser/components/contextualidentity/content/aboutContainers.js:215 (Diff revision 4) > +function createFormRow(item, item2) { > + let row = document.createElement('div'); > + row.setAttribute("class", "row"); > + row.appendChild(item); > + if (item2) { > + row.appendChild(item2); > + } ``` createFormRow(items...) { <stuff> for (let item of items) { row.appendChild(item); } } ``` ::: browser/components/contextualidentity/content/aboutContainers.js:229 (Diff revision 4) > + "chrome://browser/skin/usercontext/personal.svg", > + "chrome://browser/skin/usercontext/work.svg", > + "chrome://browser/skin/usercontext/banking.svg", > + "chrome://browser/skin/usercontext/shopping.svg", This is going to be painful... right now, all third-party themes would have to provide images here or our UI is going to look weird. But they have to provide them at these specific URLs, which is painful for third-party themes. But separately, we want users to be able to choose between these images and map them to arbitrary usercontextids. On the whole, it feels like maybe these images should be in chrome://browser/content/ rather than skin. I don't see a good way of having them be themable. But maybe Jared or Mike has a better idea. ::: browser/components/contextualidentity/content/aboutContainers.js:235 (Diff revision 4) > + "chrome://browser/skin/usercontext/work.svg", > + "chrome://browser/skin/usercontext/banking.svg", > + "chrome://browser/skin/usercontext/shopping.svg", > + ].forEach((icon) => { > + let iconSwatch = document.createElement("input"); > + iconSwatch.id = "iconbutton-" + icon.match(/([^\/]*)[.].+$/)[1]; If we're not putting this in markup for some reason, just loop over personal/work/banking/shopping, and then construct the icon URI from that? Seems more straightforward. ::: browser/components/contextualidentity/content/aboutContainers.js:245 (Diff revision 4) > + iconSwatch.checked = true; > + } > + div.appendChild(iconSwatch); > + let iconSwatchLabel = document.createElement("label"); > + iconSwatchLabel.setAttribute("for", iconSwatch.id); > + iconSwatchLabel.style.setProperty("--icon-button-image", "url('" + icon + "')"); This is another advantage to putting this in the markup: you could just have all the markup in, you know, markup, and the images pulled in from CSS, instead of this mixture... ::: browser/components/contextualidentity/content/aboutContainers.js:273 (Diff revision 4) > + let colorSwatchLabel = document.createElement("label"); > + colorSwatchLabel.setAttribute("for", colorSwatch.id); > + colorSwatchLabel.style.setProperty("--swatch-color", color); > + div.appendChild(colorSwatchLabel); These labels need text in so the items are readable for a11y users. ::: browser/locales/en-US/chrome/browser/aboutContainers.properties:7 (Diff revision 4) > +aboutContainers.colorLabel = Color > +aboutContainers.nameLabel = Color Copy/paste mistake? ::: browser/themes/shared/aboutContainers.css:12 (Diff revision 4) > - width: 100%; > - border-collapse: collapse; > + --preference-selected-color: #0996f8; > + --preference-unselected-color: #333; > + --preference-active-color: #858585; We already have in-content styles for input fields. Why can't we reuse those? ::: browser/themes/shared/aboutContainers.css:38 (Diff revision 4) > +.row:last-of-type, > +#containers > li:last-of-type { These match the same thing, right? Why both selectors? ::: browser/themes/shared/aboutContainers.css:108 (Diff revision 4) > +.description { > + display: flex; > + align-items: center; > +} > + > +.controls button { Again, feels like we should be able to reuse more in-content styling. ::: browser/themes/shared/aboutContainers.css:165 (Diff revision 4) > - border-bottom: 1px solid var(--in-content-border-color); > - margin-bottom: 1.5em; > + border-block-end: 1px solid var(--in-content-border-color); > + margin-block-end: 1.5em; Why can't this just stay border/margin-bottom? ::: browser/themes/shared/aboutContainers.css:170 (Diff revision 4) > - border-bottom: 1px solid var(--in-content-border-color); > - margin-bottom: 1.5em; > + border-block-end: 1px solid var(--in-content-border-color); > + margin-block-end: 1.5em; > + padding-block-end: 15px; > +} > + > +header h1 { There's only 1 h1 here, so the ancestor selector is not necessary. ::: browser/themes/shared/aboutContainers.css:174 (Diff revision 4) > + > +header h1 { > + font-size: 2.5rem; > + font-weight: normal; > + line-height: 40px; > + -moz-user-select: none; Not sure why we're doing this, we don't do it on e.g. about:privatebrowsing. We're contemplating doing it for everything on about:preferences, but then, shouldn't we set it somewhere else, like on <body>, and be done with it, instead of sprinkling it on individual elements? :-) ::: browser/themes/shared/aboutContainers.css:216 (Diff revision 4) > +.modal { > + min-width: 66ch; > + background-color: #fbfbfb; > + background-clip: content-box; > + color: #424e5a; > + font-size: 14px; Why are we fixing the font-size here? ::: browser/themes/shared/aboutContainers.css:256 (Diff revision 4) > + > +.close-icon { > + padding: 0; > + min-inline-size: 16px; > + min-block-size: 16px; > + background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 16, 16, 0); I like Linux, but this image doesn't exist anywhere else, so we can't just ship this across platforms.

https://reviewboard.mozilla.org/r/66878/#review64100 > Please can we just put the markup in the document and hide it, and only show it when we need to show it? Instead of the `bodyCallback` you could just use `replaceChild` to swap out bodies (and hide/unhide them). I had it like this but, it was suggested to do it this way instead. Will change it back :). > Seems like the modal should just be position: fixed? Then you won't need this. Sure :). > We should preselect and prefill these in the UI rather than putting in the defaults here. If we don't want to do that, we should ensure that you can't accept the dialog without having made choices. It's also odd that you could update an item here while not providing some of the required information, thus reverting it to the default. > > Perhaps the form fields need a sprinkling of 'required' attributes? Will do that instead thanks. > We already have in-content styles for input fields. Why can't we reuse those? They didn't match how the mockup was and how XUL about:preferences looks > These match the same thing, right? Why both selectors? One is for the form and one is for the list. No longer matches the same. > Again, feels like we should be able to reuse more in-content styling. But it doesn't quite match the mockup I was given? > Why can't this just stay border/margin-bottom? Not if we want to support all content directions easily right? > Not sure why we're doing this, we don't do it on e.g. about:privatebrowsing. We're contemplating doing it for everything on about:preferences, but then, shouldn't we set it somewhere else, like on <body>, and be done with it, instead of sprinkling it on individual elements? :-) This was a copy of XUL css for the header there, about:preferences headers are not selectable... not sure why.

Jonathan or baku, can you include screenshots of what everything looks like so far?

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66878/diff/4-5/

Is moving the svg files out of skin ok with you :baku?

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66878/diff/5-6/

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66878/diff/6-7/

(In reply to Jonathan Kingston [:jkt] from comment #35) > Is moving the svg files out of skin ok with you :baku? Sure. What does it change for me? Do we need to update any existing piece of code?

Done the changes in the patch, I actually removed all the icons and replaced it with a sprite to sort the colours out which is in Bug 1289131.

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66878/diff/7-8/

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page https://reviewboard.mozilla.org/r/66878/#review65098 ::: browser/components/contextualidentity/content/aboutContainers.js:19 (Diff revision 8) > +const defaultContainerColor = "#0996f8"; > +const colorProperties = { > + blue: "#0996f8", > + turquoise: "#01bdad", > + green: "#57bd35", > + yellow: "#ffcb00", > + orange: "#ff9216", > + red: "#d92215", > + pink: "#ea385e", > + purple: "#7a2f7a" These colors are duplicated between here and inside of the SVG. It would be better if they could be defined in only one location. Paolo has used chrome://browser/skin/filters.svg#fill along with setting the `fill` property in CSS so the sytling of the graphic can be done in CSS. For example, filter: url("chrome://browser/skin/filters.svg#fill"); fill: rgb(224, 41, 29); If you do it this way, you can define CSS variables for each color, then you can share the color between the swatch background and the usercontext icon. ::: browser/components/contextualidentity/content/aboutContainers.js:73 (Diff revision 8) > window.addEventListener("DOMContentLoaded", function load() { > document.getElementById("help-link-icon").href = > Services.urlFormatter.formatURLPref("app.support.baseURL") + "containers"; > > let containersList = document.getElementById("containers"); > - ContextualIdentityService.getIdentities().forEach(identity => { > + containersList.addEventListener('click', clickManager, false); Is this keyboard accessible? ::: browser/components/contextualidentity/content/aboutContainers.js:91 (Diff revision 8) > + display(identity); > -}); > + }); > +} > > function clickManager(event) { > + let userContextId = parseInt(event.target.closest('li').dataset.userContextId); Please use parseInt(number, 10) to make sure that parseInt uses base-10 numbers when parsing this. ::: browser/components/contextualidentity/content/aboutContainers.js:115 (Diff revision 8) > + modalOverlay.classList.remove('show'); > } > > -function remove(userContextId) { > - // TODO > +function setupModal() { > + let close = document.querySelector("#modal-header > .close-icon"); > + close.addEventListener("click", closeModal); The modal shouldn't get closed if the close button is right-clicked or middle-clicked. Please check that event.button == 0. ::: browser/components/contextualidentity/content/aboutContainers.js:132 (Diff revision 8) > + while (body.lastChild) { > + body.lastChild.remove(); > + } > + > + if (options.bodyFragment) { > + body.appendChild(options.bodyFragment); > + } > + modalOverlay.classList.add('show'); > + > + let input = body.querySelector('input'); > + if (input) { > + input.focus(); > + } If bodyFragment is undefined here, then a modal is created with nothing inside of it. Is that useful? Should bodyFragment be a required argument? ::: browser/components/contextualidentity/content/aboutContainers.js:184 (Diff revision 8) > + refreshUI(); > + event.preventDefault(); > + }); > + > + let nameLabel = document.createElement("label"); > + nameLabel.for = "name-input"; There is no special `for` property on labels. You need to use `nameLabel.setAttribute("for", "name-input");` ::: browser/components/contextualidentity/content/aboutContainers.js:196 (Diff revision 8) > + nameInput.setAttribute("maxlength", 20); > + nameInput.value = identity.name; > + form.appendChild(createFormRow('div', nameLabel, nameInput)); > + > + let iconHeading = document.createElement("h4"); > + iconHeading.for = "iconbutton"; Ditto. ::: browser/components/contextualidentity/content/aboutContainers.js:202 (Diff revision 8) > + iconHeading.textContent = containersBundle.GetStringFromName("aboutContainers.iconHeading"); > + form.appendChild(createFormRow('fieldset', iconHeading, createIconButtons(identity.icon))); > + > + let colorHeading = document.createElement("h4"); > + colorHeading.textContent = containersBundle.GetStringFromName("aboutContainers.colorHeading"); > + form.appendChild(createFormRow('fieldset', colorHeading, createColorSwatches(identity.color))); nit, please use double-quotes here as you have done elsewhere. ::: browser/components/contextualidentity/content/aboutContainers.js:207 (Diff revision 8) > + form.appendChild(createFormRow('fieldset', colorHeading, createColorSwatches(identity.color))); > + > + > + let done = document.createElement("button"); > + done.textContent = containersBundle.GetStringFromName("aboutContainers.submitButton"); > + form.appendChild(createFormRow('div', done)); Ditto. ::: browser/components/contextualidentity/content/aboutContainers.js:210 (Diff revision 8) > + let done = document.createElement("button"); > + done.textContent = containersBundle.GetStringFromName("aboutContainers.submitButton"); > + form.appendChild(createFormRow('div', done)); > + > + createModal({ > + parentElement: document.body, parentElement is unused. ::: browser/components/contextualidentity/content/aboutContainers.js:248 (Diff revision 8) > + let img = document.createElement("img"); > + img.src = `chrome://browser/content/usercontext.svg#${icon}-gray`; Please set the alt attribute to the empty string here too. ::: browser/components/contextualidentity/content/aboutContainers.js:259 (Diff revision 8) > + [ > + "blue", > + "turquoise", > + "green", > + "yellow", > + "orange", > + "red", > + "pink", > + "purple" > + ].forEach((color) => { > + let colorSwatch = document.createElement("input"); These are already defined by the colorProperties object at the beginning of this file. You can use Object.keys(colorProperties) to get an array of the key names. Please use for..of instead of .forEach, so: for (let color of Object.keys(colorProperties)) { ... } ::: browser/components/contextualidentity/content/aboutContainers.js:282 (Diff revision 8) > + let img = document.createElement("img"); > + img.src = `chrome://browser/content/usercontext.svg#circle-${color}`; Please set the alt attribuet to the empty string here. ::: browser/locales/en-US/chrome/browser/aboutContainers.properties:27 (Diff revision 8) > +aboutContainers.pink.label = Pink > +aboutContainers.purple.label = Purple > + > +aboutContainers.fingerprint.label = Fingerprint > +aboutContainers.briefcase.label = Briefcase > +aboutContainers.dollar.label = Dollar sign How is this going to translate internationally? "Dollar sign" is very North American centric. Will the graphic change per locale as well? This needs to be re-thought and is a complex problem. The locales that we ship are not geoconstrained meaning people in Great Britain may install the en-US build of Firefox, but they shouldn't see a $, instead they should see a pound sign (£). There is an international icon for currency (U+00A4) that could be used instead, or we could use a generic "money" icon and label it "Money" instead. Also, please include a localization note here to let localizers know if they should change "Dollar" sign to their nationality's currency name or not. As I said above, this problem can be made much simpler if a generic icon is used along with generic wording. ::: browser/themes/shared/aboutContainers.css:90 (Diff revision 8) > + > +.description h2 { > + font-size: 1rem; > +} > + > +.description img { .description > img ::: browser/themes/shared/aboutContainers.css:129 (Diff revision 8) > +input[type="radio"] + label { > + flex: auto; > + display: flex; > + align-items: center; > + justify-content: center; > + -moz-user-select: none; We shouldn't be adding a custom implementation of form controls to Firefox. The fact that the mockup describes a new type of form controls is a bug in the mockup. Please change this styling to use the in-content styles. You may want to bring this to the attention of the UX team so we can make sure we have a consistent design throughout the product. ::: browser/themes/shared/aboutContainers.css:198 (Diff revision 8) > + inline-size: 100vw; > + block-size: 100vh; > + position: fixed; > + offset-block-start: 0; > + offset-inline-start: 0; > + background-color: rgba(0, 0, 0, 0.5); nit, no spaces after commas in color macros. ::: browser/themes/shared/aboutContainers.css:244 (Diff revision 8) > + font-size: 14px; > +} > + > +.modal-header button { > + border: 0; > + background-color: transparent !important; Why does this need `!important`? We should be very cautious about introducing `!important` as it is very contagious. ::: browser/themes/shared/aboutContainers.css:253 (Diff revision 8) > +%ifdef XP_UNIX > +%ifndef XP_MACOSX > +%define UNIX_BUT_NOT_MAC > +%endif > +%endif > + > +%ifndef UNIX_BUT_NOT_MAC I'm sorry, I misled you here. Rather than defining a new preprocessor definition, please just use: %ifdef XP_UNIX %ifndef XP_MACOSX ... %else ... %endif

https://reviewboard.mozilla.org/r/66878/#review65098 > Is this keyboard accessible? tabbing to the button then hitting enter works for me? > How is this going to translate internationally? "Dollar sign" is very North American centric. Will the graphic change per locale as well? > > This needs to be re-thought and is a complex problem. The locales that we ship are not geoconstrained meaning people in Great Britain may install the en-US build of Firefox, but they shouldn't see a $, instead they should see a pound sign (£). There is an international icon for currency (U+00A4) that could be used instead, or we could use a generic "money" icon and label it "Money" instead. > > Also, please include a localization note here to let localizers know if they should change "Dollar" sign to their nationality's currency name or not. As I said above, this problem can be made much simpler if a generic icon is used along with generic wording. Changed to keep $ sign but added l10n note. > We shouldn't be adding a custom implementation of form controls to Firefox. The fact that the mockup describes a new type of form controls is a bug in the mockup. > > Please change this styling to use the in-content styles. You may want to bring this to the attention of the UX team so we can make sure we have a consistent design throughout the product. Not sure if the issue is the selector here just being the default. I have prefixed it in the .radio-buttons selector.

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66878/diff/8-9/

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66878/diff/9-10/

Baku: based on the feedback from :jaws I have changed the CSS being defined in the JS to just be classes, it uses variables again however defined in the CSS. I'm not sure if this brings back the perf issues (however I would hope not as the selectors won't apply to normal tabs etc). See: https://reviewboard.mozilla.org/r/66878/diff/8-10/

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page https://reviewboard.mozilla.org/r/66878/#review65400

Also, please fix the commit message on the patch. It currently reads "Bug 1267916 1289131 - add in UX for about:containers page". Mentions of bug 1289131 should be removed.

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66878/diff/10-11/

:jaws removed the bug number, it does resolve the issue there though, do commits not list numerous bugs if they are all solved by the same bug?

(In reply to Jonathan Kingston [:jkt] from comment #52) > :jaws removed the bug number, it does resolve the issue there though, do > commits not list numerous bugs if they are all solved by the same bug? We generally just close the other bug as either resolved-fixed and put in the whiteboard that it was fixed by the other bug or close it as a dupe. Depends if they are talking about different issues.

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page https://reviewboard.mozilla.org/r/66878/#review65718 ::: browser/base/content/browser.js:4102 (Diff revision 11) > * Updates the User Context UI indicators if the browser is in a non-default context > */ > function updateUserContextUIIndicator() > { > let hbox = document.getElementById("userContext-icons"); > + hbox.className = ""; This might unexpectedly remove some classes that we want to keep on it. Can you switch this to using an attribute that can be cleared without affecting other CSS usages? ::: browser/base/content/browser.js:4121 (Diff revision 11) > let label = document.getElementById("userContext-label"); > label.setAttribute('value', ContextualIdentityService.getUserContextLabel(userContextId)); > - label.style.color = identity.color; > > let indicator = document.getElementById("userContext-indicator"); > indicator.style.listStyleImage = "url(" + identity.icon + ")"; While you're here can you change this to have the styles defined within CSS instead of the style property? ::: browser/base/content/usercontext.svg:12 (Diff revision 11) > + width="32" height="32" viewBox="0 0 32 32"> > + <style> > + path, circle { > + fill: menutext; > + } > + path:not(:target), circle:not(:target) { newline after comma please ::: browser/components/contextualidentity/content/aboutContainers.js:149 (Diff revision 11) > + color: defaultContainerColor > + }; > + let title = containersBundle.GetStringFromName("aboutContainers.newContainerTitle"); > + if (userContextId) { > + identity = ContextualIdentityService.getIdentityFromId(userContextId); > + identity.name = ContextualIdentityService.getUserContextLabel(identity.userContextId); nit, trailing whitespace ::: browser/components/contextualidentity/content/usercontext.css:2 (Diff revision 11) > + --identity-tab-color: #0996f8; > + --identity-icon-color: #00a7e0; Are some of these supposed to be slightly different? ::: browser/components/contextualidentity/content/usercontext.css:59 (Diff revision 11) > } > + > +tab[usercontextid] { > + background-image: linear-gradient(to right, transparent 20%, var(--identity-tab-color) 30%, var(--identity-tab-color) 70%, transparent 80%); > + background-size: auto 2px; > + background-Repeat: no-repeat; nit, all lowercase. background-repeat ::: browser/components/customizableui/CustomizableWidgets.jsm:1155 (Diff revision 11) > - let item = doc.createelementns(knsxul, "toolbarbutton"); > - item.setattribute("label", bundle.getstring("usercontext.aboutpage.label")); > + let item = doc.createElementNS(kNSXUL, "toolbarbutton"); > + item.setAttribute("label", bundle.getString("userContext.aboutPage.label")); What patch is this based on? I don't see these in fx-team tip. ::: browser/locales/en-US/chrome/browser/aboutContainers.properties:27 (Diff revision 11) > +# LOCALIZATION NOTE (aboutContainers.dollar.label) > +# String represents a money sign but currently uses a dollar sign so don't change to local currency > +aboutContainers.dollar.label = Dollar sign Pleae file a bug to fix this. ::: browser/locales/en-US/chrome/browser/aboutContainers.properties:27 (Diff revision 11) > +# LOCALIZATION NOTE (aboutContainers.dollar.label) > +# String represents a money sign but currently uses a dollar sign so don't change to local currency > +aboutContainers.dollar.label = Dollar sign Pleae put the bug number here too. ::: browser/themes/shared/aboutContainers.css:76 (Diff revision 11) > +.description, .controls { > + flex: 1; > } > > .description { > + flex: 2; .description gets flex:1 here but then four lines later gets flex:2 ::: browser/themes/shared/aboutContainers.css:85 (Diff revision 11) > .description { > + flex: 2; > font-weight: bold; > - font-size: 1.17em; > - width: 100%; > + min-inline-size: 29ch; > + margin-inline-end: auto; > + word-break: break-all; Why do we want to break words in hal f? The default, 'normal', should be fine. ::: browser/themes/shared/aboutContainers.css:125 (Diff revision 11) > + display: flex; > + align-items: center; > +} > + > +.controls > button { > + background: white; Should this background adjust with the CSS variable color for the border? Or does white always work here? ::: browser/themes/shared/aboutContainers.css:151 (Diff revision 11) > + -moz-user-select: none; > + outline: 2px solid transparent; > + outline-offset: 4px; > + -moz-outline-radius: 100%; > + margin-inline-end: 15px; > + vertical-align: text-top; vertical-align generally only works on display:table-cell. Are you sure this is necessary here? ::: browser/themes/shared/aboutContainers.css:186 (Diff revision 11) > - border-bottom: 1px solid var(--in-content-border-color); > - margin-bottom: 1.5em; > + border-block-end: 1px solid var(--in-content-border-color); > + margin-block-end: 1.5em; > + padding-block-end: 15px; > } > > header #help-link-icon { You can drop the `header ` from this, as if there is only one #help-link-icon on the page then it should always be in the header, right? ::: browser/themes/shared/aboutContainers.css:194 (Diff revision 11) > - min-height: 24px; > + min-block-size: 16px; > background-image: url("chrome://global/skin/in-content/help-glyph.svg#help"); > + background-size: contain; > } > > header #help-link-icon:hover { Ditto. ::: browser/themes/shared/aboutContainers.css:215 (Diff revision 11) > +#modal-overlay.show { > + display: flex; > +} Instead of using a 'show' className, just set the [hidden] attribute on #modal-overlay and remove it when you want to show the overlay. Then you can get rid of the display:none; in the #modal-overlay rule above. ::: browser/themes/shared/aboutContainers.css:253 (Diff revision 11) > +.modal-header > button:enabled { > + border: 0; > + background-color: transparent; > +} Does this button get disabled? How do you want it to look when it's disabled? If it doesn't get disabled, then you should remove the :enabled psuedo class. ::: browser/themes/shared/aboutContainers.css:264 (Diff revision 11) > +%ifndef XP_LINUX > + .close-icon { > + background-image: -moz-image-rect(url("chrome://global/skin/icons/close.png"), 0, 20, 20, 0); > + background-size: contain; > + } Were you going to move the .close-icon styles out to a separate file that can be included? We shouldn't be duplicating these styles.

https://reviewboard.mozilla.org/r/66878/#review65718 > Are some of these supposed to be slightly different? Yup will ask Bram if we can change them to be the same. > Why do we want to break words in hal > f? The default, 'normal', should be > fine. Because long words in container names broke the layout for small window sizes where the user has put in just letters and no spaces. > Does this button get disabled? How do you want it to look when it's disabled? If it doesn't get disabled, then you should remove the :enabled psuedo class. The enabled is to game the selector specificity of another one, it's instead of !important. I can probably select .modal > instead, > Were you going to move the .close-icon styles out to a separate file that can be included? We shouldn't be duplicating these styles. Ah I can specifically just for linux sure :)

https://reviewboard.mozilla.org/r/66878/#review65718 > Yup will ask Bram if we can change them to be the same. Yeah will ask bram if we can make these the same. > What patch is this based on? I don't see these in fx-team tip. From baku "bug 1267916 - 2 patches; bug 1287091 - 4 patches; then another patch, that is what I'm writing: https://pastebin.mozilla.org/8885740"

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66878/diff/11-12/

https://reviewboard.mozilla.org/r/66878/#review66412 ::: browser/themes/shared/aboutContainers.css:266 (Diff revision 12) > + background-color: transparent; > +} > + > +%if defined(XP_MACOSX) || defined(XP_WIN) > + .close-icon { > + background-image: -moz-image-rect(url("chrome://global/skin/icons/close.png"), 0, 20, 20, 0); This needs testing in both MAC and Windows, my suspicion is it won't work.

Comment on attachment 8770908 [details] [diff] [review] patch 1 - Open about Containers in any existing menu (except context menu) Review of attachment 8770908 [details] [diff] [review]: ----------------------------------------------------------------- Carrying over review from https://bugzilla.mozilla.org/show_bug.cgi?id=1267916#c3 as all points are addressed.

Comment on attachment 8770908 [details] [diff] [review] patch 1 - Open about Containers in any existing menu (except context menu) Review of attachment 8770908 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/locales/en-US/chrome/browser/browser.properties @@ +702,5 @@ > userContextBanking.accesskey = B > userContextShopping.accesskey = S > userContextNone.accesskey = N > > +userContext.aboutPage.label = Manage containers This needs to be capital case, "Manage Containers". See the title of about:addons, "Add-ons Manager"

Comment on attachment 8774395 [details] Bug 1267916 - add in UX for about:containers page I am going to hold off on reviewing this for now, we are talking about changing directions to implement this as a subdialog within about:preferences. Much of this code can be reused and much of the boilerplate code will not be necessary. See bug 1283314 for more details.

I suspect I can cancel this NI, right?

Comment on attachment 8770908 [details] [diff] [review] patch 1 - Open about Containers in any existing menu (except context menu) Obsolete based upon a patch I pushed, this has been changed to load about:containers#preferences

Comment on attachment 8772402 [details] [diff] [review] patch 2 - about:containers This is moving to about:preferences#containers so no longer needed.

Comment on attachment 8784758 [details] Bug 1267916 - Open about preferences #containers in any existing menu (except context menu) https://reviewboard.mozilla.org/r/74088/#review71992 Minor issues everywhere, so I think this needs another round of review, sorry. I'm away Friday-Monday, so if you're in a hurry best ask :jaws or someone else. Sorry! Also... does about:preferences#containers already exist? Or is that a separate patch/bug? Or...? ::: browser/base/content/browser-context.inc:72 (Diff revision 1) > <menu id="context-openlinkinusercontext-menu" > label="&openLinkCmdInContainerTab.label;" > accesskey="&openLinkCmdInContainerTab.accesskey;" > hidden="true"> > <menupopup oncommand="gContextMenu.openLinkInTab(event);" > - onpopupshowing="return gContextMenu.createContainerMenu(event);" /> > + onpopupshowing="return gContextMenu.createContainerMenu(event, true);" /> The second parameter is ignored, so I don't think this is necessary? ::: browser/base/content/browser-sets.inc:96 (Diff revision 1) > <command id="cmd_gestureRotateRight" oncommand="gGestureSupport.rotate(event.sourceEvent)"/> > <command id="cmd_gestureRotateEnd" oncommand="gGestureSupport.rotateEnd()"/> > <command id="Browser:OpenLocation" oncommand="openLocation();"/> > <command id="Browser:RestoreLastSession" oncommand="restoreLastSession();" disabled="true"/> > <command id="Browser:NewUserContextTab" oncommand="openNewUserContextTab(event.sourceEvent);" reserved="true"/> > + <command id="Browser:OpenAboutContainers" oncommand="switchToTabHavingURI('about:preferences#containers', true);" reserved="true"/> There is no key combination for this, so the reserved keyword is useless and should be omitted. The dangers of copy-paste... ;-) ::: browser/components/customizableui/CustomizableWidgets.jsm:1114 (Diff revision 1) > + if (!item.hasAttribute("usercontextid")) { > + win.switchToTabHavingURI("about:preferences#containers", true); > + return; > + } > + If you set command="Browser:OpenAboutcontainers" on the menuitem, do you still need this? Because I don't think so, and that would reduce the code duplication going on here... ::: browser/components/customizableui/CustomizableWidgets.jsm:1115 (Diff revision 1) > let items = doc.getElementById("PanelUI-containersItems"); > > let onItemCommand = function (aEvent) { > let item = aEvent.target; > + if (!item.hasAttribute("usercontextid")) { > + win.switchToTabHavingURI("about:preferences#containers", true); This (and the same code in the `<command>` element's handler) doesn't switch to an existing preferences tab (which it can then switch to the right pane). You want to use the `openPreferences` helper instead.

Comment on attachment 8784758 [details] Bug 1267916 - Open about preferences #containers in any existing menu (except context menu) https://reviewboard.mozilla.org/r/74088/#review72072 These changes look fine to me but this patch can't land without the containers section of about:preferences. ::: browser/base/content/utilityOverlay.js:437 (Diff revision 2) > let menuitem = document.createElement("menuitem"); > menuitem.setAttribute("usercontextid", "0"); > menuitem.setAttribute("label", bundle.getString("userContextNone.label")); > menuitem.setAttribute("accesskey", bundle.getString("userContextNone.accesskey")); > > // We don't set an oncommand/command attribute attribute because if we have s/attribute attribute/attribute/

Comment on attachment 8784758 [details] Bug 1267916 - Open about preferences #containers in any existing menu (except context menu) https://reviewboard.mozilla.org/r/74088/#review72990 Clearing the review request until the other parts of the patches are uploaded.

jkt, any updates on this? I thought there was an urgency to get this landed. I haven't seen the patch that will implement this in about:preferences. Please ask for help if you are stuck on something.

Hey jaws, Sorry I have been silently working on this. Will push what I have tomorrow as it's just a few styling tweaks away :).

Comment on attachment 8789411 [details] Bug 1267916 - add in UX for about:preferences#containers page https://reviewboard.mozilla.org/r/77648/#review77074 Why are there changes to browser/base/jar.mn, browser/base/content/browser.js, and browser/base/content/utilityOverlay.js in this patch? ::: browser/components/contextualidentity/content/usercontext.css:75 (Diff revision 1) > +tab[usercontextid] { > + background-image: linear-gradient(to right, transparent 20%, var(--identity-tab-color) 30%, var(--identity-tab-color) 70%, transparent 80%); > + background-size: auto 2px; > + background-repeat: no-repeat; How should this look for pinned tabs? Also, this type of change doesn't belong in this patch since this patch should be focused on implementing the about:preferences controls. Including various other changes makes the patch harder to review as well means that it is harder to test and back-out if need be. ::: browser/components/contextualidentity/content/usercontext.css:90 (Diff revision 1) > + background-image: var(--identity-icon); > + filter: url(chrome://browser/skin/filters.svg#fill); > + fill: var(--identity-icon-color); > + background-size: contain; > + background-repeat: no-repeat; > + background-position: left center; This looks like it will break in right-to-left text locales. ::: browser/components/preferences/containers.js:99 (Diff revision 1) > + iconElement.className = "icon"; > + iconElement.setAttribute("data-identity-icon", icon); Do we ever have an iconElement with the className of "icon" but not the "data-identity-icon" attribute? Or vice versa? It seems that we can drop the class attribute here. If we really do need it, then we need a more specific name because "icon" is too generic and we'll need to write selectors such as `[data-identity-icon].icon` to make sure that we are only selecting the contextual identity icons. ::: browser/components/preferences/in-content/containers.js:19 (Diff revision 1) > + document.getElementById("backContainersLink").addEventListener("click", function () { > + window.location = "about:preferences#privacy"; > + return false; > + }); Why does this return false here? We shouldn't change window.location but instead use gotoPref("privacy"); ::: browser/components/preferences/in-content/containers.js:27 (Diff revision 1) > + _rebuildView: function () > + { > + const containers = ContextualIdentityService.getIdentities(); > + while (this._list.firstChild) { > + this._list.firstChild.remove(); > + } > + for (let container of containers) { > + let item = document.createElement("richlistitem"); > + item.setAttribute("containerName", ContextualIdentityService.getUserContextLabel(container.userContextId)); > + item.setAttribute("containerIcon", container.icon); > + item.setAttribute("containerColor", container.color); > + item.setAttribute("userContextId", container.userContextId); > + > + this._list.appendChild(item); > + } > + }, When I first loaded this view I didn't see any icons for the entries though the markup contained what looked to be correct: <xul:hbox xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" xbl:inherits="data-identity-icon=containerIcon,data-identity-color=containerColor" xmlns:xbl="http://www.mozilla.org/xbl" height="24" width="24" class="icon" data-identity-icon="chrome://browser/skin/usercontext/work.svg" data-identity-color="#f89c24"/> Looking deeper in to the issue, we don't use work.svg anymore but have switched this to "briefcase". Are you missing a change to update the database? ::: browser/components/preferences/in-content/containers.js:55 (Diff revision 1) > + }, > + onPeferenceClick: function (button) { > + this.openPreferenceDialog(button.getAttribute("value")); > + }, > + > + onAddButtonClick: function (button) { nit, please use the this style instead: onAddButtonClick(button) { ... }, here and elsewhere throughout your patch. ::: browser/components/preferences/in-content/containers.js:73 (Diff revision 1) > + const params = { userContextId : userContextId, > + identity : identity, > + windowTitle : title }; nit, this can be written more succinctly: const params = {userContextId, identity, windowtitle: title}; If the local variable has the same name as the property you don't need to specify it twice. ::: browser/components/preferences/in-content/privacy.js:441 (Diff revision 1) > gSubDialog.open("chrome://browser/content/preferences/permissions.xul", > null, params); > }, > > /** > + * Displays fine-grained, per-site preferences for tracking protection. This comment is incorrect. ::: browser/components/preferences/in-content/privacy.js:670 (Diff revision 1) > + > + // don't override pref value in UI > + return undefined; Functions by default return undefined, so I'm not sure what this is necessary for. What does it mean to not override pref value in UI? ::: browser/locales/en-US/chrome/browser/preferences/containers.dtd:7 (Diff revision 1) > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + > +<!ENTITY label.label "Name"> > +<!ENTITY addButton.label "Add New Container"> > +<!ENTITY backLink.label "&lt;&lt; Go Back to Privacy"> This should use &laquo; instead of two &lt; ::: browser/locales/en-US/chrome/browser/preferences/containers.properties:14 (Diff revision 1) > +containers.nameLabel = Name: > +containers.namePlaceholder = Enter a container name > +containers.submitButton = Done > +containers.iconHeading = Icon: > +containers.updateContainerTitle = %S Container Preferences > +containers.confirmRemove = Are you sure you would like to delete The button says Remove but the text here says "delete". We shouldn't show a dialog here. In the "Allowed Sites - Add-ons Installation" dialog we don't show a modal when an allowed site is removed. We may want to show a dialog here if there are sites open that are using the container about to be removed, but otherwise we shouldn't show a dialog. Also, asking "Are you sure you would like to delete" can make it seem that this is a dangerous operation. You can watch https://drive.google.com/open?id=0B8DUY8PKjIjDWjJyUjB4empaTUk for more details about why I dislike this text. Further, the dialog, if needed, should not use "OK" / "Cancel" buttons but "Remove" and "Cancel". ::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd:113 (Diff revision 1) > <!ENTITY browserContainersHeader.label "Container Tabs"> > <!ENTITY browserContainersLearnMore.label "Learn more"> > <!ENTITY browserContainersEnabled.label "Enable Container Tabs"> > <!ENTITY browserContainersEnabled.accesskey "n"> > +<!ENTITY browserContainersSettings.label "Settings…"> > +<!ENTITY browserContainersSettings.accesskey "s"> This accesskey is already in use by the "Show Cookies..." button when History is set to "Use custom settings" ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm (Diff revision 1) > - // inline style is only a temporary fix for some bad performances related > - // to the use of CSS vars. This code will be removed in bug 1278177. > if (!tab.hasAttribute("usercontextid")) { > - tab.style.removeProperty("background-image"); > - tab.style.removeProperty("background-size"); > - tab.style.removeProperty("background-repeat"); Should these changes actually be in bug 1278177?

Comment on attachment 8784758 [details] Bug 1267916 - Open about preferences #containers in any existing menu (except context menu) https://reviewboard.mozilla.org/r/74088/#review77100 Please resolve the issues as you fix them. This patch is still showing as 2 unresolved issues.

Comment on attachment 8789411 [details] Bug 1267916 - add in UX for about:preferences#containers page https://reviewboard.mozilla.org/r/77648/#review77074 > How should this look for pinned tabs? > > Also, this type of change doesn't belong in this patch since this patch should be focused on implementing the about:preferences controls. > > Including various other changes makes the patch harder to review as well means that it is harder to test and back-out if need be. This is the current appearence just not generated CSS anymore, it should be a highlight above the tab. Moving to a different patch though :). > Do we ever have an iconElement with the className of "icon" but not the "data-identity-icon" attribute? Or vice versa? > > It seems that we can drop the class attribute here. > > If we really do need it, then we need a more specific name because "icon" is too generic and we'll need to write selectors such as `[data-identity-icon].icon` to make sure that we are only selecting the contextual identity icons. Ah sorry I had to change the code to realise why this was coded like this. It's more that I'm letting the data attr cascade using custom properties which removing the .icon from the CSS makes the color apply to the wrong element in the menu elsewhere (in this case the menuitem). ``` <menuitem data-identity-icon="circle"> <xul:hbox> <xul:image class="menu-iconic-icon"/> </xul:hbox> </menuitem> ``` ``` <hbox data-identity-icon="circle" class="icon"/> ``` I can however change the icon class name to be more specific like container-icon. > nit, please use the this style instead: > > onAddButtonClick(button) { > ... > }, > > here and elsewhere throughout your patch. I left browser/components/preferences/in-content/privacy.js it feels bad practice to change a files style so I kept consistent with it. I can file a follow up bug to change that perhaps?

This patch now depends on the patch in: Bug 1278177.

Can you generate an interdiff between the patch that I previously reviewed and the new one uploaded? Mozreview normally does this automatically but it looks like something was done to the patch so it's not marked as a new revision of the previous patch.

Looks like that worked :jaws. needinfo'ing for your attention. Thanks.

Visiting https://reviewboard.mozilla.org/r/79250/diff/1-2/ shows a blank diff. Did you see something different?

Tried both id's I think it lost the link the the last set of reviews when it decided to obsolete the patches. I guess the only alternative would be manually diffing the squashed old diff: https://reviewboard.mozilla.org/r/66876/diff/raw/ and the new one: https://reviewboard.mozilla.org/r/79250/diff/3/raw/ Mozreview seems pretty confused by it though.

Comment on attachment 8791975 [details] Bug 1267916 - add in UX for about:preferences#containers page https://reviewboard.mozilla.org/r/79250/#review78242 I used BeyondCompare to compare the raw patch from attachment 8789411 [details] (obsolete) to the new patch in attachment 8791975 [details] and it says that they are Binary Same. Do you remember what you changed between revisions? Do you see those changes here?

Sorry Jared, I missed your comment. I have attached an export of the patch in my repo the only bits changed were what you asked for as well as splitting out the coloring of the icons with CSS into the seperate bug which needs to be applied as a patch first before this one. (I will be available tomorrow if you have any questions :) ) Thanks!

This is still very difficult for me to see what has changed. I downloaded this new bugzilla-hosted attachment and compared it with the latest patch hosted on mozreview and I don't see much difference, with the exception that the bugzilla-hosted diff includes 8 lines of context and the mozreview-hosted diff has 3 lines of context, meaning that creating an interdiff on my machine of these two files is very noisy. A quick scan of the changes doesn't show any difference though. Can you help me more here?

This is the last snapshot from mozreview I was able to find that works, you can see the changed code to the latest patch(https://bugzilla.mozilla.org/attachment.cgi?id=8798365&action=edit) is: - usercontext.css only has one selector due to the code removed to the other bug. - Other code has been removed for the same reason: usercontext.svg, utilityOverlay.js, CustomizableWidgets.jsm, browser.js - Changes to containers.dtd to use laquo - No removal of other icons as again in the other bug

See comment above, this patch is the old one however shows the differences between the last review and the latest patch. Thanks for your patience with this.

Hey Jared, I'm not sure if you have seen this, I know you are pretty busy. Would you be able to do another review? Feel free to ping me if there are other issues. Thanks

Comment on attachment 8798365 [details] [diff] [review] Bug 1267916 - add in UX for about:preferences#containers page r?jaws Review of attachment 8798365 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the long delay on reviewing this. I was able to compare the changes with your latest upload, so that helped a lot. Thanks! ::: browser/themes/shared/preferences/containers.css @@ +47,5 @@ > +radiogroup > radio[focused=true] { > + outline-color: var(--preference-selected-color); > +} > + > +radiogroup > radio:active { Do we have radio elements outside of radiogroups? If not, then we can simplify all of these selectors to remove the `radiogroup > ` part. Also, this one in particular should be `radio:hover:active` because we usually don't apply the :active styling unless the cursor is also hovering the element.

Comment on attachment 8805948 [details] Bug 1267916 - Open about preferences #containers in any existing menu (except context menu) r+ from :jaws this is just a rebased version.

Comment on attachment 8806434 [details] Bug 1267916 - add in UX for about:preferences#containers page r+ from :jaws this is just a rebased version.

jaws: could you fix the review in mozreview too, so that we can use autoland to land this changes ? thanks!

Comment on attachment 8805948 [details] Bug 1267916 - Open about preferences #containers in any existing menu (except context menu) https://reviewboard.mozilla.org/r/89540/#review89768

Please hold fire checking this in until I have fixed the tests of the dependent work. Thanks!

Comment on attachment 8806434 [details] Bug 1267916 - add in UX for about:preferences#containers page https://reviewboard.mozilla.org/r/89864/#review89772 r=me with the issues below addressed. ::: browser/components/preferences/containers.js:59 (Diff revision 1) > + }, > + > + uninit() { > + }, > + > + // Check if name string as to if the form can be submitted Can we get rid of this function and just set required=true on the name field? ::: browser/components/preferences/containers.js:61 (Diff revision 1) > + const name = document.getElementById("name"); > + const btnApplyChanges = document.getElementById("btnApplyChanges"); we don't normally mark references to elements as const based on how they're used here. Especially since you're later changing attributes of btnApplyChanges. ::: browser/components/preferences/containers.js:63 (Diff revision 1) > + > + // Check if name string as to if the form can be submitted > + checkForm() { > + const name = document.getElementById("name"); > + const btnApplyChanges = document.getElementById("btnApplyChanges"); > + if (!!name.value) { I think this will cause an eslint error for violating http://eslint.org/docs/rules/no-extra-boolean-cast ::: browser/components/preferences/containers.js:146 (Diff revision 1) > + return radiogroup; > + }, > + > + onApplyChanges() { > + let icon = document.getElementById("icon").value; > + let color = document.getElementById("color").value; Should we have any kind of input validation on the icon and color here? ::: browser/components/preferences/in-content/privacy.js:484 (Diff revision 1) > > /** > + * Displays container panel for customising and adding containers. > + */ > + showContainerSettings() { > + window.location = "about:preferences#containers"; Can we use goToPref here instead of changing window.location?

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/127379f03ad8 part 1 - Open about preferences #containers in any existing menu (except context menu), r=jaws https://hg.mozilla.org/integration/mozilla-inbound/rev/964223d37a2e part 2 - Add in UX for about:preferences#containers page, r=jaws

Comments applied and patch landed. There was only 1 issue in the comments: I didn't remove checkForm() because otherwise the 'Done' button is always shown as clickable. Plus, I don't see 'required' as one of the attributes of the textbox XUL element.

https://hg.mozilla.org/mozilla-central/rev/127379f03ad8 https://hg.mozilla.org/mozilla-central/rev/964223d37a2e

Tanvi - my understanding is that this prefs work, like the rest of the containers UI, should not ride the trains in 52.

(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #103) > Tanvi - my understanding is that this prefs work, like the rest of the > containers UI, should not ride the trains in 52. Right, this is conditional on the Containers pref (privacy.userContext.enabled) which is enabled in Nightly only and will not ride the trains.