Closed
Bug 1434584
Opened 6 years ago
Closed 6 years ago
Video and Audio control sharing icon are impossible to read with default mac theme
Categories
(Firefox :: Site Permissions, defect, P1)
Tracking
()
VERIFIED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox58 | --- | wontfix |
firefox59 | --- | wontfix |
firefox60 | --- | verified |
firefox61 | --- | verified |
People
(Reporter: jya, Assigned: johannh)
Details
Attachments
(3 files)
44.19 KB,
image/png
|
Details | |
1.70 MB,
video/mp4
|
Details | |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details |
The default macOS High Sierra appearance configuration is to use "dark menu bar and Dock" (can be changed in System Preferences -> General" When you make a WebRTC call, two icons are added in the menu bar that allows to see that your microphone and camera are being shared. However, those icons are black. At first I didn't even know there were icons, only when I started another app that placed another icon did I realise as it seemed the new icon was hanging in the middle of the menu bar. In fact they were offset by Firefox control sharing icon which are invisible unless you click on them. See screen capture attached.
Reporter | ||
Comment 1•6 years ago
|
||
on the right of the video control icon, there's an audio one. But you can't tell, it just looks like an empty gap
Comment 2•6 years ago
|
||
A short video showing the same thing.
Comment 3•6 years ago
|
||
Looks rather critical to fix, and probably to uplift. jib, jesup, what do you think?
Rank: 15
Component: General → WebRTC: Audio/Video
Flags: needinfo?(rjesup)
Flags: needinfo?(jib)
Priority: -- → P1
Product: Firefox → Core
Comment 5•6 years ago
|
||
Probably should nominate for tracking-59
Comment 6•6 years ago
|
||
Maire, I think changing icons is probably outside of the webrtc teams space. Who would be a good contact who might be able to help us with this?
Flags: needinfo?(mreavy)
Comment 7•6 years ago
|
||
Unfortunately these icons are still PNG files. I was hoping we could convert them to SVG so that they would work better for situations with a hidpi and a lowdpi screen. Seems to be another reason to switch to SVG, but we'll probably need a short term fix with PNG icons using a lighter color, and select which icon we show based on the OS version. Johann, any chance you could help with this?
Component: WebRTC: Audio/Video → Device Permissions
Flags: needinfo?(jhofmann)
Product: Core → Firefox
Comment 8•6 years ago
|
||
Sounds from comment 7 that this is not a regression, but I agree it might be worth an uplift if the fix is trivial? I'm still on Sierra. Is the High Sierra dark theme a new thing? Is ESR affected?
status-firefox58:
--- → wontfix
status-firefox59:
--- → affected
status-firefox60:
--- → affected
status-firefox-esr52:
--- → ?
Flags: needinfo?(jib)
Assignee | ||
Comment 9•6 years ago
|
||
ESR is not affected. I can look into this.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Flags: needinfo?(jhofmann)
Assignee | ||
Comment 10•6 years ago
|
||
So we _are_ using an SVG for this nowadays, and that's the problem. It seems like context-fill does not work in the OSX system status bar, which makes the image black by default. We could just replace the context-fill images with SVGs that have their colors hard coded, but I'm not sure how to switch between status item images to reflect the either dark or white status bar background. Reading this SO answer, it seems like the proper way to do this is to use the native cocoa APIs to set a "template image": https://stackoverflow.com/questions/24623559/nsstatusitem-change-image-for-dark-tint#24644754 Markus, do you have any thoughts on what might be best here?
Flags: needinfo?(mstange)
Comment 11•6 years ago
|
||
The patch in bug 1403989 can probably be made to work for the system status bar case as well - it'll cause us to mark images as "template images" automatically for images that only use black colors with different alpha values. I don't remember what was missing in that patch, or why I didn't put it up for review, but it should be easy to finish up.
Flags: needinfo?(mstange)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•6 years ago
|
||
Markus, as far as I can see StandaloneNativeMenu is only used by SystemStatusBarCocoa which in turns only seems to be used by our WebRTC indicators right now, so I didn't really think it was necessary to add a parameter or an attribute indicating that the icon is a template image. Let me know if you'd prefer to have that, though.
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8963999 [details] Bug 1434584 - Make icon in MacOS nsStandaloneNativeMenu a template image. https://reviewboard.mozilla.org/r/232822/#review238478 Makes sense. Thanks!
Attachment #8963999 -
Flags: review?(mstange) → review+
Comment 16•6 years ago
|
||
Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bacd19ec0d4b Make icon in MacOS nsStandaloneNativeMenu a template image. r=mstange
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bacd19ec0d4b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Assignee | ||
Comment 18•6 years ago
|
||
Comment on attachment 8963999 [details] Bug 1434584 - Make icon in MacOS nsStandaloneNativeMenu a template image. Approval Request Comment [Feature/Bug causing the regression]: Apple introducing a dark menu bar [User impact if declined]: WebRTC sharing indicator in the menubar is invisible (see attachment 8947077 [details]) [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: It works on Nightly for me [Needs manual test from QE? If yes, steps to reproduce]: Open a video or audio stream on OSX with a dark top menu bar and assert that the indicator is shown. [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Not really [Why is the change risky/not risky?]: It's a short patch where we are simply setting an image attribute to a menu icon that is only used for WebRTC indicators [String changes made/needed]: None
Attachment #8963999 -
Flags: approval-mozilla-beta?
Updated•6 years ago
|
Comment 19•6 years ago
|
||
Comment on attachment 8963999 [details] Bug 1434584 - Make icon in MacOS nsStandaloneNativeMenu a template image. Fix an invisible icon for dark OSX menus. Approved for 60.0b11.
Attachment #8963999 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 20•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/82edf3649c7c
Updated•6 years ago
|
Flags: qe-verify+
Comment 21•6 years ago
|
||
I managed to reproduce the bug using an older version of Nightly (2018-01-31) on macOS 10.13. The link I used is: https://mozilla.github.io/webrtc-landing/gum_test.html. I retested everything using the latest Nightly 61.0a1 and beta 60.0b11 on the same platform and the bug is not reproducing anymore.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•