1278177 - Containers should use CSS vars and not inline tab styling.

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

We should revert bug 1273628 when we have a good solution about bad performances related to CSS var.

Comment 1

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1278177 - Change container icons to a single sprite sheet and to be rendered by CSS

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

Comment 2

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8791411 [details]
Bug 1278177 - Change container icons to a single sprite sheet and to be rendered by CSS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78822/diff/1-2/

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8791411 [details]
Bug 1278177 - Change container icons to a single sprite sheet and to be rendered by CSS

https://reviewboard.mozilla.org/r/78822/#review77608

r=me with the following issues fixed and after you have tested with ForceRTL and made sure that what you're doing with the background-position is what you intended.

::: browser/components/contextualidentity/content/usercontext.css:75
(Diff revision 2)
>  
>  #userContext-icons {
>    -moz-box-align: center;
>  }
> +
> +tab[usercontextid] {

.tabbrowser-tab[usercontextid]

::: browser/components/contextualidentity/content/usercontext.css:82
(Diff revision 2)
> +  background-size: auto 2px;
> +  background-repeat: no-repeat;
> +}
> +
> +.menuitem-iconic[data-usercontextid] > .menu-iconic-left > .menu-iconic-icon,
> +toolbarbutton[usercontextid] > .toolbarbutton-icon,

.toolbarbutton-1[usercontextid] > .toolbarbutton-icon

::: browser/components/contextualidentity/content/usercontext.css:90
(Diff revision 2)
> +  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;

Have you tested this with the Force-RTL add-on?

Comment 4

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8791411 [details]
Bug 1278177 - Change container icons to a single sprite sheet and to be rendered by CSS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78822/diff/2-3/

Comment 5

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8791411 [details]
Bug 1278177 - Change container icons to a single sprite sheet and to be rendered by CSS

https://reviewboard.mozilla.org/r/78822/#review77608

> .toolbarbutton-1[usercontextid] > .toolbarbutton-icon

Used .subviewbutton instead of .toolbarbutton-1 as it's for the menus that the container button opens.

> Have you tested this with the Force-RTL add-on?

Tested with Force-RTL but set the position to be center center anyway which I ended up losing from transfering the patch over to this bug.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f52a805df83
Change container icons to a single sprite sheet and to be rendered by CSS r=jaws

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser_sessionStoreContainer.js:

https://hg.mozilla.org/integration/autoland/rev/e4e51a8149ba5e54054545481f7f9d576880f70d

Push following that one has browser-chrome tests which shows the error: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8c3384531cc0c11e25f3fd6f3860e00a2fc8e3c7
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=6009698&repo=autoland

[task 2016-11-02T12:06:29.912658Z] 12:06:29     INFO - Entering test bound 
[task 2016-11-02T12:06:29.914676Z] 12:06:29     INFO - TEST-PASS | browser/components/sessionstore/test/browser_sessionStoreContainer.js | "1" == 1 - 
[task 2016-11-02T12:06:29.920986Z] 12:06:29     INFO - TEST-PASS | browser/components/sessionstore/test/browser_sessionStoreContainer.js | The docShell has the correct userContextId - 1 == 1 - 
[task 2016-11-02T12:06:29.922590Z] 12:06:29     INFO - Leaving test bound 
[task 2016-11-02T12:06:29.924086Z] 12:06:29     INFO - Entering test bound test
[task 2016-11-02T12:06:29.925967Z] 12:06:29     INFO - TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_sessionStoreContainer.js | Uncaught exception - at resource://gre/modules/ContextualIdentityService.jsm:284 - TypeError: identity is undefined
[task 2016-11-02T12:06:29.931857Z] 12:06:29     INFO - Stack trace:
[task 2016-11-02T12:06:29.933655Z] 12:06:29     INFO -     setTabStyle@resource://gre/modules/ContextualIdentityService.jsm:284:5
[task 2016-11-02T12:06:29.935250Z] 12:06:29     INFO -     addTab@chrome://browser/content/tabbrowser.xml:2159:15

Comment 8

[Carsten Book [:Tomcat]]

also caused failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6014237&repo=autoland

Comment 9

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fac489d4f2e

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8791411 [details]
Bug 1278177 - Change container icons to a single sprite sheet and to be rendered by CSS

https://reviewboard.mozilla.org/r/78822/#review90172

::: browser/base/content/browser.js:4133
(Diff revision 3)
>      hbox.hidden = true;
>      return;
>    }
>  
>    let identity = ContextualIdentityService.getIdentityFromId(userContextId);
> +  hbox.setAttribute("data-identity-color", identity.color);

there is null check just after this line. You should set the attribute only if identity is not null.

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:282
(Diff revision 3)
> -      tab.style.removeProperty("background-repeat");
>        return;
>      }
>  
>      let userContextId = tab.getAttribute("usercontextid");
>      let identity = this.getIdentityFromId(userContextId);

this can return null.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab12ade75727
Change container icons to a single sprite sheet and to be rendered by CSS r=jaws

Comment 12

[Phil Ringnalda (:philor)]

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