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)

x86_64
macOS
defect

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)

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.
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
Attached video invisible.mp4
A short video showing the same thing.
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
Agreed
Rank: 15 → 5
Flags: needinfo?(rjesup)
Probably should nominate for tracking-59
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)
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
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?
Flags: needinfo?(jib)
ESR is not affected. I can look into this.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Flags: needinfo?(jhofmann)
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)
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)
Thanks, Johann for taking this one.
Flags: needinfo?(mreavy)
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 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+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bacd19ec0d4b
Make icon in MacOS nsStandaloneNativeMenu a template image. r=mstange
https://hg.mozilla.org/mozilla-central/rev/bacd19ec0d4b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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?
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+
Flags: qe-verify+
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.