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

RESOLVED FIXED in Firefox 53

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years 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])

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

3 years 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

3 years 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
Reporter

Comment 2

3 years 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

3 years 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

3 years 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

3 years 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 :-).
Reporter

Comment 8

3 years 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

3 years 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

3 years 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

3 years ago
Attachment #8814446 - Flags: review?(florian)
Assignee

Comment 14

3 years 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

3 years 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

3 years 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

3 years ago
Assignee: nobody → sajarvis
Reporter

Updated

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

Updated

3 years ago
Attachment #8814525 - Flags: review?(florian)
Assignee

Comment 18

3 years 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

3 years 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 21

3 years 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
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

3 years 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

3 years 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)

Comment 27

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7636e8dc84d6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.