Use SVG icons for the global sharing indicator on Windows/Linux

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Device Permissions
P3
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: florian, Assigned: sajarvis, Mentored)

Tracking

unspecified
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [fxprivacy][good first bug])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Bug 1274480 added glyphs for the camera, microphone and screen icons in chrome://browser/skin/glyphs.svg

I think we can remove the remaining files in https://hg.mozilla.org/integration/fx-team/file/a1558d048e76/browser/themes/shared/webrtc and replace them with the SVG version.

Updated

a year ago
Whiteboard: [fxprivacy][triage]
Priority: -- → P3
Whiteboard: [fxprivacy][triage] → [fxprivacy]
This seems like a mentored bug or even a good first bug, right?
Mentor: florian@queze.net
(Reporter)

Comment 2

a year ago
The file that needs to be edited is http://searchfox.org/mozilla-central/source/browser/themes/windows/webRTC-indicator.css

It would be a good idea to merge the windows and linux versions of this file into a single file in the browser/themes/shared/ folder.
Whiteboard: [fxprivacy] → [fxprivacy][good first bug]
How can I reproduce and where to view the result of the css files changed by me.
(Reporter)

Comment 4

a year ago
You can only reproduce this on Windows or Linux, the UI is different on Mac.

To make the global sharing indicator appear, you need to use a page that will request access to the camera/microphone/screen, eg. https://mozilla.github.io/webrtc-landing/gum_test.html

Comment 5

9 months ago
So basically what i need to do is replace these files with their svg version.And delete the two files one from windows folder and another from linux folder and put that file in shared folder with the reference changed to svg version.
Am i right florian?
Thanks
(Reporter)

Comment 6

9 months ago
(In reply to rgargrajatgarg from comment #5)
> So basically what i need to do is replace these files with their svg
> version.And delete the two files one from windows folder and another from
> linux folder and put that file in shared folder with the reference changed
> to svg version.
> Am i right florian?

That's right :-).
Comment hidden (mozreview-request)
(Reporter)

Comment 8

9 months ago
mozreview-review
Comment on attachment 8798552 [details]
Bug 1279533 Replaced the files in browser/themes/shared/webrtc with their svg versions and merged linux and windows webRTC-indicator.css in shared folder changing the reference to svg files.

https://reviewboard.mozilla.org/r/83994/#review82576

Using SVG, we don't need 2 different files for camera-white-16 and camera-white-16@2x, an SVG icon can scale gracefully. We also no longer need a different file to show the icon in a different color.

See for example http://searchfox.org/mozilla-central/rev/76609a05d6ef7ba4223ed79e479c73fb2543a107/browser/themes/shared/notification-icons.svg#61 used at http://searchfox.org/mozilla-central/rev/76609a05d6ef7ba4223ed79e479c73fb2543a107/browser/themes/shared/notification-icons.inc.css#122

The bootstrap.py and patchfile files seem to have been added by accident here.
Attachment #8798552 - Flags: review?(florian)
Comment hidden (mozreview-request)
(Reporter)

Comment 10

9 months ago
mozreview-review
Comment on attachment 8798552 [details]
Bug 1279533 Replaced the files in browser/themes/shared/webrtc with their svg versions and merged linux and windows webRTC-indicator.css in shared folder changing the reference to svg files.

https://reviewboard.mozilla.org/r/83994/#review82586

I'm not sure if my previous comment made this clear, but the patch should not create new svg files.
Attachment #8798552 - Flags: review?(florian)
Comment hidden (mozreview-request)
(Reporter)

Comment 12

9 months ago
mozreview-review
Comment on attachment 8798552 [details]
Bug 1279533 Replaced the files in browser/themes/shared/webrtc with their svg versions and merged linux and windows webRTC-indicator.css in shared folder changing the reference to svg files.

https://reviewboard.mozilla.org/r/83994/#review82594

When removing chrome files, you need to stop packaging them. The package manifest for the files you removed is at http://searchfox.org/mozilla-central/rev/76609a05d6ef7ba4223ed79e479c73fb2543a107/browser/themes/shared/jar.inc.mn#133

::: browser/themes/shared/webRTC-indicator.css:32
(Diff revision 3)
> +#firefoxButton:hover {
> +  background-color: #f2f2f2;
> +}
> +
> +#screenShareButton {
> +  background-image: url("webRTC-screen-16.svg");

The url here should be something like chrome://browser/skin/notification-icons.svg#screen-sharing

We shouldn't create new svg files here. You may need to add a few lines to notification-icons.svg though.
Attachment #8798552 - Flags: review?(florian)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8814446 - Flags: review?(florian)
(Assignee)

Comment 14

7 months ago
Hey all, first time here but think I navigated the wikis all right and came up with a patch for this. Let me know if I missed anything (technical or otherwise). Happy to be here! Thanks.

The patch was tested on Linux. My understanding is the "try server" will build cross-platform, is that right?
(Reporter)

Comment 15

7 months ago
mozreview-review
Comment on attachment 8814446 [details]
Bug 1279533 - Use SVG sharing icons on Linux and Windows.

https://reviewboard.mozilla.org/r/95676/#review95724

Thanks, these changes look good. One last thing to handle: we want to keep the white color of these icons (the current patch turns them into red, which isn't very visible above an orange background). You can do that in notification-icons.svg, the same way the #*-sharing icons are colored. Maybe use camera-indicator, microphone-indicator, etc... for the ids.
Attachment #8814446 - Flags: review?(florian) → review-
(Reporter)

Comment 16

7 months ago
(In reply to Steve Jarvis [:sajarvis] from comment #14)
> Hey all, first time here but think I navigated the wikis all right and came
> up with a patch for this.

Welcome here, great start for a first patch! :-)

> The patch was tested on Linux. My understanding is the "try server" will
> build cross-platform, is that right?

Our build infrastructure will build on all platforms if we push to the try server or once we check in the patch. The try server won't be very useful in this case because automated tests don't cover the appearance of these icons.
(Reporter)

Updated

7 months ago
Assignee: nobody → sajarvis
(Reporter)

Updated

7 months ago
Attachment #8798552 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8814525 - Flags: review?(florian)
(Assignee)

Comment 18

7 months ago
mozreview-review
Comment on attachment 8814446 [details]
Bug 1279533 - Use SVG sharing icons on Linux and Windows.

https://reviewboard.mozilla.org/r/95674/#review95760

::: browser/themes/shared/notification-icons.svg:45
(Diff revision 2)
>      }
> +
> +    #camera-indicator,
> +    #microphone-indicator,
> +    #screen-indicator {
> +      fill: white;

I know the #\*-sharing color is defined with RGB, but the included file (icon-colors.inc.svg) uses 'black' and 'white' for fill definitions, so it defining white here seemed most consistent.
(Reporter)

Comment 19

7 months ago
mozreview-review
Comment on attachment 8814525 [details]
Bug 1279533 - Color the active sharing indicators white.

https://reviewboard.mozilla.org/r/95736/#review95932

Looks good and works well, thanks! :-)
Attachment #8814525 - Flags: review?(florian) → review+
(Reporter)

Comment 20

7 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebab26f0002c4d6757d4c22158c3ac420c02a774
Bug 1279533 - Use SVG sharing icons on Linux and Windows. r=florian
(Reporter)

Comment 21

7 months ago
Congratulations for your first bug fixed! If you would be interested in other bugs about removing files we no longer need, you may want to have a look at the list of bugs marked as dependencies of bug 1316187. For example bug 1319762.
Blocks: 1316187

Comment 22

7 months ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2a6721fbbde
Backed out changeset ebab26f0002c
backed this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=670ad168fb708a7eca83480419f6206833b2566e because of continued test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=39930510&repo=mozilla-inbound

maybe this need a try run to find out what was going wrong
Flags: needinfo?(sajarvis)
(Assignee)

Comment 24

7 months ago
(In reply to Carsten Book [:Tomcat] from comment #23)
> backed this out in
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=670ad168fb708a7eca83480419f6206833b2566e because of
> continued test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=39930510&repo=mozilla-
> inbound
> 
> maybe this need a try run to find out what was going wrong
 
Judging by the #developers IRC there were a handful of changes backed out. The linked failure is a network timeout error, and I can't imagine how this chageset might have caused that. 

I don't yet have access to the try server, I was planning on requesting that once this bug is resolved. Florian, how should I proceed?
Flags: needinfo?(sajarvis) → needinfo?(florian)
(Reporter)

Comment 25

7 months ago
The failures are related to bug 1319854, I'll reland soon, don't worry the failures aren't related to your patch :-).
Flags: needinfo?(florian)
(Reporter)

Comment 26

7 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7636e8dc84d63a73436a9f7c5142a2db9c9e06da
Bug 1279533 - Use SVG sharing icons on Linux and Windows. r=florian

Comment 27

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7636e8dc84d6
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.