Repository: electron/electron (language: cpp)

Issue:
[Bug]: Dark pink hover/focus background for menus in menu bar
### Preflight Checklist

- [X] I have read the [Contributing Guidelines](https://github.com/electron/electron/blob/main/CONTRIBUTING.md) for this project.
- [X] I agree to follow the [Code of Conduct](https://github.com/electron/electron/blob/main/CODE_OF_CONDUCT.md) that this project adheres to.
- [X] I have searched the [issue tracker](https://www.github.com/electron/electron/issues) for a bug report that matches the one I want to file, without success.

### Electron Version

19.0.8

### What operating system are you using?

Windows

### Operating System Version

Windows 10 build 19044

### What arch are you using?

x64

### Last Known Working Electron version

Electron v17.4.11

### Expected Behavior

Standard menu bar menus should use a neutral gray hover/focus background (this used to be the case in the past).

### Actual Behavior

Not clear why, but it seems that standard menus in electron now (at least on my machine) have a dark pink hover/focus background. Not sure where this is being picked up from. None of my other apps on Windows use this colour, and it's certainly not part of my OS-wide theme.

I get this when running the vanilla quick-start example https://github.com/electron/electron-quick-start

![screenshot from the quick-start application, showing the dark pink hover/focus background on an open menu](https://user-images.githubusercontent.com/895831/179403341-c5149475-e909-4578-82a9-bb22740fc35b.png)

first spotted on this electron-based application I sometimes contribute to https://github.com/ThePacielloGroup/CCAe/issues/251

### Testcase Gist URL

from @jediwade https://gist.github.com/eb8cfee3a6ae8fd88bb808f8b12e5f62

### Additional Information

_No response_


Discussion / hints:
On the off-chance that it might be picking up some old/outdated registry values, I had to dive into regedit, but couldn't find anything relating to this odd pink colour.

`Computer\HKEY_CURRENT_USER\Control Panel\Colors MenuHiLight` is set to a vibrant blue:
![value for Computer\HKEY_CURRENT_USER\Control Panel\Colors MenuHiLight](https://user-images.githubusercontent.com/895831/179403976-60504d33-cb78-405d-b613-a58e7c9cb88d.png)

`Computer\HKEY_CURRENT_USER\Control Panel\Colors Menu` is set to a warm-ish gray:
![value for Computer\HKEY_CURRENT_USER\Control Panel\Colors Menu](https://user-images.githubusercontent.com/895831/179403986-b4fe8253-aa04-41df-a0a7-3b6127289e70.png)

overall theme for Windows is set to use cool blue

![screenshot of the windows settings for personalisation > colour, showing selected cool blue color](https://user-images.githubusercontent.com/895831/179404119-2339145c-4770-41b5-9157-feda008556fb.png)

I experience this as well using Windows 11 Pro (21H2, 22000.832) on an x64 system. While testing another bug, I found out that the color was normal (light gray color) when using Electron v17.4.11. Electron versions 18-21.alpha have this weird dark pink color for the file menu.

https://gist.github.com/eb8cfee3a6ae8fd88bb808f8b12e5f62
thanks for confirming @jediwade ... was wondering if this was just some problem on my specific windows setup.

seeing this happen now on some recent electron-based apps being distributed...and it's rather unsightly.
No problem @patrickhlauke. I thought I had some weird setting in Windows 11 that made that happen for me. I spent a good bit of time just trying to find an app already on my computer with that classic file menu just to see if other apps were also pink or if it was just Electron apps. If you can, you may want to copy my gist link above and edit the original bug report to include that link to see if that helps move this along faster.
This is still present in Electron versions v21.2.0 and v22.0.0-alpha 7.

Running up-to-date Windows 11 (version 22H2, build 22621.675).
This problem of pink menu buttons is still present in Electron versions v21.2.2 and v22.0.0-beta.3

Running up-to-date Windows 11 (version 22H2, build 22621.755).
Problem is still present in Electron version v22.0.0.

Running Windows 11 Pro (version 22H2, build 22621.819).
would be nice if the team could at least acknowledge the bug...

@patrickhlauke I imagine they have to carefully pick and choose which bugs they want to work on, and the ones with titles suggesting app crashing/breaking experiences probably come first. Because this is technically just a cosmetic annoyance that can be worked around by creating your own file menu system in HTML/CSS/JS or using an existing package (I know I've seen one in the past, but can't recall the name) to accomplish the same thing. Not sure if you read the Electron blog or not, but they have stated that [December will be a quiet month](https://www.electronjs.org/blog/a-quiet-place-22) to give maintainers a break and only major security updates will be released. So we know it will be January at the earliest for a fix now.

For my own app, I've focused on bug fixes and adding new features rather than implementing my own file menu system. Some might argue that I should anyways so my app could have a more unified look.

I've mainly continued commenting here to keep the issue ticket alive (and not going stale or whatever the proper GitHub verbiage is) with relevant information about testing done. If I had the time (and an inkling of where to start), I might try to have my first PR with Electron to fix it.
Problem is still present in electron v23.0.0-alpha.3

Running Windows 11 Pro, 22H2, 22621.963
> was wondering if this was just some problem on my specific windows setup.

It likely is, yes, otherwise there would be far more complaints.

Please understand that issues like this which reproduce on only a handful of systems are near impossible for Electron maintainers to track down, and takes significant effort for what is gained.

If the issue readily reproduces for you, the most impactful thing you can do to get this issue resolved is to track down which version it first reproduces in. [Electron Fiddle](https://www.electronjs.org/fiddle) is the easiest way to do that since you can quickly run any version of Electron. There's also a bisect helper you can run which will save you some searching time. You'll want to try to track it down to nightlies or alpha/beta releases as well if you can.
@dsanders11 this isn't a problem just on @patrickhlauke's computer. I experience this problem as well and I have not done some heavy customization of my Windows computer that would be outside the norm of a regular Windows user (light/dark mode choice, color preference, etc).

You are assuming that just because more people aren't complaining about this issue that it doesn't exist. For all we know, most Electron developers don't even use the native file menu and create their own in HTML/CSS/JS, use a library that does that for them, or don't even have a file menu at all.

As this issue ticket documents, I have already done the work you are asking for. The last version of Electron to not have pink menus is v17.4.11 and the problem first appears in final release v18.

If you absolutely have to know which nightly, alpha, or beta to warrant this ticket being worked on, I will try to do that tonight.

This problem happens on Windows 10 and 11.
@dsanders11 The issue first appears in Electron v18.0.0-beta.1. Because there are no builds for v18.0.0-alpha.6, I was not able to test it and I do not know how to compile Electron. The v18.0.0 alphas I tested (1, 3, and 5) were all good.
The same error occurs on my computer running Windows 10 Pro (21H2, Build 19044.2486). I also did not make any unusual adjustments to the Windows display settings (basically just switched to dark mode, but it occurs in light mode as well).
@tkleinke I can confirm that Windows 11 in Dark Mode does the same with a dark pink background color on click.

@dsanders11 When looking at a difference between v18.0.0-alpha.5 and v18.0.0-beta.1, you can see the following changes in electron/shell/browser/ui/views/menu_bar.cc:

![image](https://user-images.githubusercontent.com/9596313/215279719-3b5759ab-6d75-4a56-83ee-a67d9b35d335.png)
![image](https://user-images.githubusercontent.com/9596313/215279756-f0fa47a6-c111-4bc3-9889-1707a17b33d3.png)

According to [this comment](https://github.com/electron/electron/issues/1250#issuecomment-209545788) from several years ago, they were able to change the menu bar color to black by changing the value of "background_color_". I understand there is a slight difference in that the commenter changed the default color vs the active/pressed color of menu buttons, but could this be the same area of code to look into? If I am reading this correctly, it is also a change that was inherited from a Chromium bump. Since Chromium doesn't have a true file menu system, this could just be an accident from Chromium devs that they never see or maybe Electron has a mismatched color reference somewhere that the GetBackground()-get_color() is returning an unexpected color.
> You are assuming that just because more people aren't complaining about this issue that it doesn't exist.

I wasn't being dismissive, I apologize if it came across that way. My initial take was just that it was a niche issue because of the lack of reports, and not having noticed it in my own usage on Windows. Those kinds of issues definitely exist, especially on Windows - the last one I tracked down had 5+ users reporting it and it ended up being caused by a registry key from a 15 year old version of Adobe Photoshop on those machines.

I've dug deeper into this issue and was able to reproduce locally on Windows. I always run in dark mode and it's much less noticeable as a result, so I hadn't taken notice of it myself in the past.

> @dsanders11 When looking at a difference between v18.0.0-alpha.5 and v18.0.0-beta.1, you can see the following changes in `electron/shell/browser/ui/views/menu_bar.cc`:

Thanks for digging into this further. 👍

The root cause looks to be a race condition with `views::CreateThemedSolidBackground`. The background color isn't set immediately in that case, leaving the possibility of retrieving the color before it is set. From [the Chromium source code](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/background.cc;l=113-114;drc=89905883cfe77bd00528b842827a6fcda488a3bb):

```
// ThemedSolidBackground is a solid background that stays in sync with a view's
// ColorProvider.
```

[Chromium initializes unset colors to solid red](https://source.chromium.org/chromium/chromium/src/+/main:ui/gfx/color_palette.h;l=12-14;drc=3297fb6d70a02d8c4af8db2bc9e315053744442b) to make them easier to spot:

```c++
// A placeholder value for unset colors. This should never be visible and is red
// as a visual flag for misbehaving code.
constexpr SkColor kPlaceholderColor = SK_ColorRED;
```

So that's where the the dark pink color is coming from, it's the solid red color with the opacity applied for the focus background.

I've tested a couple of different ways to fix this locally, but need to do further testing on Linux to make sure it doesn't break anything there. This is an area where we unfortunately don't have test coverage (visual stuff like menu bars are tricky to test robustly).
@dsanders11 I apologize as well. I can certainly understand that there are dozens, if not hundreds, of issue tickets that come in that are the result of a highly customized system, for super old versions of Electron (or other software as you mentioned) that I doubt are even supported, or simply a misunderstanding of how an API should work.

You're welcome on taking the dive. I did spend an hour or two trying to comb through Chromium source code to try and figure out the problem, but I'm not familiar with C++ and didn't have much luck.

Because Chromium caused the issue, does an issue ticket (or PR once you are able to fix it) need to be made with Chromium?

Thank you for looking into this. Let me know if there is anything else I can do to help.
> Because Chromium caused the issue, does an issue ticket (or PR once you are able to fix it) need to be made with Chromium?

I don't believe so. It's ultimately due to our usage of the internal Chromium APIs, in `electron/shell/browser/ui/views/menu_bar.cc`, as you saw.

> Thank you for looking into this. Let me know if there is anything else I can do to help.

Since it's readily reproducible I think we're good. 👍 

Fix patch:
diff --git a/shell/browser/ui/views/menu_bar.cc b/shell/browser/ui/views/menu_bar.cc
index 7a0ba8139e1f0..0ec33726417b3 100644
--- a/shell/browser/ui/views/menu_bar.cc
+++ b/shell/browser/ui/views/menu_bar.cc
@@ -35,12 +35,11 @@ const char MenuBar::kViewClassName[] = "ElectronMenuBar";
 MenuBar::MenuBar(NativeWindow* window, RootView* root_view)
     : background_color_(kDefaultColor), window_(window), root_view_(root_view) {
   const ui::NativeTheme* theme = root_view_->GetNativeTheme();
-  RefreshColorCache(theme);
-  UpdateViewColors();
 #if BUILDFLAG(IS_WIN)
   SetBackground(views::CreateThemedSolidBackground(ui::kColorMenuBackground));
-  background_color_ = GetBackground()->get_color();
 #endif
+  RefreshColorCache(theme);
+  UpdateViewColors();
   SetFocusBehavior(FocusBehavior::ALWAYS);
   SetLayoutManager(std::make_unique<views::BoxLayout>(
       views::BoxLayout::Orientation::kHorizontal));
@@ -209,6 +208,14 @@ void MenuBar::ButtonPressed(size_t id, const ui::Event& event) {
   menu_delegate->AddObserver(this);
 }
 
+void MenuBar::ViewHierarchyChanged(
+    const views::ViewHierarchyChangedDetails& details) {
+  views::AccessiblePaneView::ViewHierarchyChanged(details);
+#if BUILDFLAG(IS_WIN)
+  background_color_ = GetBackground()->get_color();
+#endif
+}
+
 void MenuBar::RefreshColorCache(const ui::NativeTheme* theme) {
   if (theme) {
 #if BUILDFLAG(IS_LINUX)
@@ -217,6 +224,8 @@ void MenuBar::RefreshColorCache(const ui::NativeTheme* theme) {
         gtk::GetFgColor("GtkMenuBar#menubar GtkMenuItem#menuitem GtkLabel");
     disabled_color_ = gtk::GetFgColor(
         "GtkMenuBar#menubar GtkMenuItem#menuitem:disabled GtkLabel");
+#elif BUILDFLAG(IS_WIN)
+    background_color_ = GetBackground()->get_color();
 #endif
   }
 }
diff --git a/shell/browser/ui/views/menu_bar.h b/shell/browser/ui/views/menu_bar.h
index 91546fb33cc55..b4beb95a723e9 100644
--- a/shell/browser/ui/views/menu_bar.h
+++ b/shell/browser/ui/views/menu_bar.h
@@ -50,6 +50,9 @@ class MenuBar : public views::AccessiblePaneView,
                                     ElectronMenuModel** menu_model,
                                     views::MenuButton** button);
 
+  void ViewHierarchyChanged(
+      const views::ViewHierarchyChangedDetails& details) override;
+
  private:
   // MenuDelegate::Observer:
   void OnBeforeExecuteCommand() override;


Test patch: