Closed
Bug 1278177
Opened 8 years ago
Closed 8 years ago
Containers should use CSS vars and not inline tab styling.
Categories
(Core :: DOM: Security, defect, P1)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: baku, Assigned: jkt)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [domsecurity-backlog][usercontextId])
Attachments
(1 file)
We should revert bug 1273628 when we have a good solution about bad performances related to CSS var.
Updated•8 years ago
|
Whiteboard: [domsecurity-backlog]
Updated•8 years ago
|
Blocks: ContextualIdentity
Whiteboard: [domsecurity-backlog] → [domsecurity-backlog][usercontextId]
Updated•8 years ago
|
Priority: -- → P4
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jkt
Comment hidden (mozreview-request) |
Comment 3•8 years ago
|
||
mozreview-review |
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?
Attachment #8791411 -
Flags: review?(jaws) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 years ago
|
||
mozreview-review-reply |
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.
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f52a805df83
Change container icons to a single sprite sheet and to be rendered by CSS r=jaws
Keywords: checkin-needed
Comment 7•8 years ago
|
||
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
Flags: needinfo?(jkt)
Comment 8•8 years ago
|
||
also caused failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6014237&repo=autoland
Assignee | ||
Comment 9•8 years ago
|
||
Reporter | ||
Comment 10•8 years ago
|
||
mozreview-review |
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•8 years ago
|
||
Pushed by amarchesini@mozilla.com:
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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(jkt)
You need to log in
before you can comment on or make changes to this bug.
Description
•