1260482 - Replace device icons with SVG versions

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Ryan Feeley [:rfeeley]]

Currently our device icons as used in Synced Tabs are bitmaps, but should be vector SVG.

Comment 1

[Stephen Horlander [:shorlander]]

I looked into doing this but I ran into a problem. 

Instead of being styled with CSS and getting a device specific class or something; it looks like it is getting the icon from  https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/SyncedTabs.jsm#56

I think we should add a class name to the item-title-container div or to its container div and then reference the image with CSS. Then we can add the SVG image and any other variants (e.g. inverted icons for the selected/focused state).

Comment 2

[Mark Hammond [:markh] [:mhammond]]

SyncedTabs.jsm does indeed specify the names of the image files, which the SyncedTabs sidebar uses to set the backgroundImage style on the element showing the image. IIUC, the reason we tend to show images this way is for themes - if we displayed images in the "normal" way (eg, <img src=...>, themes (ie, css) would be not able to override the image shown.

However, this is a bit of a problem for svg images where we want to use CSS to change stuff inside the svg itself - you can't apply additional styling to the svg itself when it's specified in CSS rather than directly in the markup (and is the same problem bug 1228478 has - it wants to use a svg but use CSS to change one of the colors in the SVG, so would need to change to using a traditional image).

So IIUC, the tension is that if we show the svg in a theme-friendly way, we can't use CSS to style the svg. I'm inclined to say "tough luck to themes - they just can't change the image shown (but would be able to style how it is shown).

Assuming that's correct, I think the change we could make here would be something like:

<div class="device-image-container">
  <img class="device-image device-image-mobile" src="chrome://browser/skin/sync-mobileIcon.svg">
  <img class="device-image device-image-desktop" src="chrome://browser/skin/sync-desktopIcon.svg">
</div>

with css something like:
.device-image-container[devicetype="mobile"] .device-image-desktop {
  display: none;
}

.device-image-container[devicetype="desktop"] .device-image-mobile {
  display: none;
}

.device-image {
 ... styling we want to apply to both svgs...
}

and the JS code setting the "devicetype" attribute on the container based on the device type (duh!).  Additional styling could also be applied.

Gijs, does the above all sound correct and reasonable?

Comment 3

[:Gijs (he/him)]

Referencing hardcoded paths into /skin/ from /content/ is kind of frowned upon though, because then 3rd party themes have to provide files at those paths.

If we want to change the SVG colors etc., can we use the techniques from bug 1054016 to still keep them as references from CSS instead?

Comment 4

[Stephen Horlander [:shorlander]]

(In reply to Mark Hammond [:markh] from comment #2)
> So IIUC, the tension is that if we show the svg in a theme-friendly way, we
> can't use CSS to style the svg. I'm inclined to say "tough luck to themes -
> they just can't change the image shown (but would be able to style how it is
> shown).

I don't want to use CSS to style the SVG directly in this case. I want to be able to change the icon based on its device type AND based whether or not it has a different state.

For example on the disclosure arrow the default image we call is:

.item.client .item-twisty-container {
  width: 16px;
  height: 16px;
  background-image: url("chrome://global/skin/tree/arrow-disclosure.svg#arrow-disclosure-expanded");
}

But when it is in a focused state we need to use the inverted version:

.item.client.selected:focus .item-twisty-container {
  background-image: url("chrome://global/skin/tree/arrow-disclosure.svg#arrow-disclosure-expanded-inverted");
}

We can't do that for the device icon because there is no class differentiating between desktop and mobile.


> Assuming that's correct, I think the change we could make here would be
> something like:
> 
> <div class="device-image-container">
>   <img class="device-image device-image-mobile"
> src="chrome://browser/skin/sync-mobileIcon.svg">
>   <img class="device-image device-image-desktop"
> src="chrome://browser/skin/sync-desktopIcon.svg">
> </div>

I think something like that would work. But I was thinking more about just adding the device-image-* class to the parent like:

<div class="item client device-image-mobile" role="option" tabindex="-1" id="item-something" title="iPhone">
  <div class="item-title-container">
    <div class="item-twisty-container"></div>
    <div class="item-icon-container" style="background-image: url(&quot;chrome://browser/skin/sync-mobileIcon.png&quot;);"></div>
    <p class="item-title">iPhone</p>
  </div>
</div>

Then we could style it with CSS like:

.item.client.device-image-mobile > .item-title-container > .item-icon-container {
  background-image: url("chrome://browser/skin/sync-mobileIcon.svg#icon");
}

.item.client.device-image-mobile > .item-title-container.selected:focus > .item-icon-container {
  background-image: url("chrome://browser/skin/sync-mobileIcon.svg#icon-inverted");
}

Comment 5

[:Gijs (he/him)]

The approach in comment #4 is fine by me as long as we put all the references to the images in CSS files (rather than inline style attributes), which I think should be possible? Or am I missing something?

Comment 6

[Stephen Horlander [:shorlander]]

Mark, how does the approach in comment #4 and comment #5 sound to you?

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to Stephen Horlander [:shorlander] from comment #6)
> Mark, how does the approach in comment #4 and comment #5 sound to you?

SGTM! Moving to the Sync component so it appears on our backlog.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

> (In reply to Stephen Horlander [:shorlander] from comment #6)
> SGTM! Moving to the Sync component so it appears on our backlog.

oh - it remains assigned to Stephen, so that probably wasn't necessary - Stephen, please unassign yourself if you don't intend doing the actual work.

Comment 9

[Stephen Horlander [:shorlander]]

Unassigned ;)

Comment 10

[Edouard Oger [:eoger]]

Stephen do you still have the SVGs?

Comment 11

[Stephen Horlander [:shorlander]]

Attachment: sync-mobileIcon.svg

Comment 12

[Stephen Horlander [:shorlander]]

Attachment: sync-desktopIcon.svg

Comment 13

[Stephen Horlander [:shorlander]]

(In reply to Edouard Oger [:eoger] from comment #10)
> Stephen do you still have the SVGs?

Attached!

Comment 14

[Edouard Oger [:eoger]]

Are these SVGs for OSX only or for all platforms?

Comment 15

[Stephen Horlander [:shorlander]]

(In reply to Edouard Oger [:eoger] from comment #14)
> Are these SVGs for OSX only or for all platforms?

For all platforms for right now. I am doing some separate places icon updates for Windows. I might roll something into that.

Comment 16

[Edouard Oger [:eoger]]

Attachment: bug-1260482.patch

- I kept the svg declaration in the osx/ windows/ linux/ jar files instead of the shared one since we're gonna have platforms specific versions at some point (see comment 15)
- I didn't remove the png versions of these icons since they are still used in about:sync-tabs, we can open up a follow up bug later if needed.

Comment 17

[Edouard Oger [:eoger]]

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

Comment 18

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8749746 [details] [diff] [review]
bug-1260482.patch

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

I think you need to change the synced tabs menu at https://dxr.mozilla.org/mozilla-central/rev/e5a10bc7dac4ee2453d8319165c1f6578203eac7/browser/components/customizableui/CustomizableWidgets.jsm#483 too? Also, if it's not too painful, can we use the svgs on about:sync-tabs too?

Comment 19

[Edouard Oger [:eoger]]

Attachment: bug-1260482.patch

Mark the Synced Tabs Panel UI never had device icons, or maybe I didn't understand what you are saying?
I also replaced/deleted the pngs by their svg equivalent in about:sync-tabs.

Comment 20

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8750416 [details] [diff] [review]
bug-1260482.patch

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

Sorry for the confusion about the panel - I was looking at the tab, not the device. This LGTM, thanks.

Comment 21

[Carsten Book [:Tomcat]]

has problems to apply:

adding 1260482 to series file
renamed 1260482 -> bug-1260482.patch
applying bug-1260482.patch
patching file services/sync/modules/SyncedTabs.jsm
Hunk #2 FAILED at 77
1 out of 2 hunks FAILED -- saving rejects to file services/sync/modules/SyncedTabs.jsm.rej

could you take a look, thanks!

Comment 22

[Edouard Oger [:eoger]]

Attachment: bug-1260482.patch

Rebased, sorry about that

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9e428c5047b3

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e428c5047b3