Support Windows 10/11 Dark Mode #16908
Conversation
…t officially "dark"
|
Very great feature. While I've heard that the native controls used by wxWidget have incomplete support for dark mode, this still makes sense. Would it be possible to add a config item to configure which mode we are in: auto, light and dark. |
|
I have not tested it. But what about the following windows:
|
Thank you for pointing this out! I have looked into those windows and fixed some of them. I will add the updated code soon. |
…re correctly in dark mode while the system is in dark mode.
See test results for failed build of commit 7dbadc3ffc |
Hi @CyrilleB79, my fork of NVDA that I'm editing doesn't have the create portable copy and COM reg fixing tool in the tools menu of NVDA settings. When I install the latest version of NVDA, these dialogs are there. How could I update my local version using the installer? |
|
There are instructions to produce NVDA's installer locally, or you can just download the installer from appVeyor artifacts generated by this PR and use it to upgrade your local copy. Though, if you do not wish to modify your local copy, you may also just tweak NVDA's code for test only, to force these dialogs to appear, modifying some if condition in |
|
Thank you! |
Thanks!
Done! |
See test results for failed build of commit 9a40244b77 |
|
That's great, but I'm having some second thoughts about the placement of the "Colour Theme" setting, maybe it would be better to put it in the "General" category or at the top of the "Visual" category? There are also windows that are not covered, such as the About dialogue box. There are also problems with the display of controls such as checkbox lists, add-on Store details controls, buttons under the mouse pointer, etc. There should be a section in the user guide about "colour theme" configuration. Developer's guide or add-on developer's guide can also be added. |
|
As @hwf1324, I wonder if there would be a global way to enable dark mode rather than calling
But the question of add-ons should be considered in any case. Also should (and can) dark mode configuration be profile aware? Re the location of the option, I'd say that the Vision panel is the most suitable, but I have no objection if other people want to put it rather in General or in another panel. The Vision panel is used to configure visual aids, and I think that choosing a color theme can be considered one, even if it is discutable. It's worth noting that the screen curtain is not a visual aid in my view, and having it in the Vision panel creates confusion: the screen curtain does not help visually impaired people to use the screen / the sight, but it's a privacy feature. Anyway, touching screen curtain is off-topic for this PR. If you stick with keeping the option in the vision panel, I wonder if it is mandatory that options in this panel be vision providers (see focus highlighter or screen curtain implementation). More specifically I wonder:
|
seanbudd
added
the
blocked/needs-product-decision
WalkthroughThe changes introduce a new color theme feature to the NVDA application, allowing users to select between light and dark themes. This includes the implementation of a new configuration option for color themes, enhancements to the user interface for managing settings, and the addition of a dark mode feature. Additionally, various UI elements and settings dialogs have been updated to improve user interaction and accessibility. Changes
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (2)
user_docs/en/userGuide.md (2)
2466-2466: Ensure clarity and adherence to style.The description for the "Enable Highlighting" checkbox should be clear and follow the one-sentence-per-line style.
Note that checking and unchecking the "Enable Highlighting" checkbox will also change the state of the other checkboxes accordingly. Therefore, if "Enable Highlighting" is off and you check this checkbox, the other checkboxes will also be checked automatically. If you only want to highlight the focus and leave the navigator object and browse mode checkboxes unchecked, the state of the "Enable Highlighting" checkbox will be half-checked.
2483-2490: Ensure clarity and adherence to style.The new section on color themes should be clear and follow the one-sentence-per-line style.
You can control whether NVDA dialogs use a light background (the "Light" theme) or a dark background (the "Dark" theme). The default theme is "Auto", which causes NVDA to match your operating system's "dark mode" setting. You can try out a different theme by changing the selected theme and then pressing the "Apply" button. This will cause the color of the settings dialog to update immediately.
|
In the past week, I figured out how to change the text color in the add-on store without breaking TextCtrl's elsewhere, and came up with a compromise for CheckListCtrl's that preserves legibility. Here are some updated screenshots:
|
There was a problem hiding this comment.
In the past week, I figured out how to change the text color in the add-on store without breaking TextCtrl's elsewhere, and came up with a compromise for CheckListCtrl's that preserves legibility. Here are some updated screenshots:
Glad to see this, not sure if there's any point to it yet, but this seems to be the place to set the details style and it seems a bit special.
|
With the disabled CheckListCtrl style, the text is not contrasted enough. Of course this is a non-issue.
It would be much better if the default style of CheckListCtrl could be like this.
In addition to the selected styles |
|
One more question I wonder if this will be resolved? It flashes the original style when opening the GUI. I wonder if it's possible to wait for the style to finish changing before showing it. |
|
@TristanBurchett it looks like this is coming along very nicely. Just a couple of questions.
|
seanbudd
added
the
conceptApproved
|
Sorry I've been quiet on this for a while -- school has kept me busy. Anyway, here's a status update.
Yeah, I understand why you're concerned about potential breakage here. Unfortunately, styling radio boxes is surprisingly difficult. Here's what I've learned.
If I ever manage to figure out radioboxes then I think checkbox lists will also work, because checkbox lists are also "static controls". Scrollbars are a different matter though -- styling scrollbars requires doing shadowing system DLL symbols on startup. I don't think that kind of low-level trick feels warranted here, or particularly safe.
The colors look like the colors that most apps use when dark mode is enabled (e.g. windows settings dialogs, chrome, vscode). Apparently there are no "standard" colors, and every app is expected to do everything on its own. They're easy to change if you want.
I'm confused about this because I'm literally using the same widget type as every other settings dialog checkbox that I could find. In particular, I'm calling wx.CheckBox().
I went ahead and reverted those changes to the dialogs.
Sure, no problem. I want to find a way to get unblocked on the radiobox issue first though. Otherwise it sounds like all of this will have been a waste of time? |
|
@TristanBurchett thanks for the status update. If you want help with this, please don't hesitate to ask and we'll try our best. CC @seanbudd |
Are you sure that's right? The PR you linked was merged 2 years ago, and wxPython 4.2.2 is using the latest wxWidgets release. My understanding is that we are now using a wxPython build with that wxWidgets commit, especially since #17181 . There may be implementation issues remaining in wxPython? Have you re-tested this since merging in master? #17181 was merged since the last time you worked on this.
I recently did some research on this, I'd like to suggest a more specific colour pallete once this is closer to being ready |
seanbudd
removed
the
blocked/needs-product-decision
|
Hey @TristanBurchett - have you managed to test with the latest master merged in? are there any other outlying issues? |
|
@seanbudd Your message gave me hope! So I tested it again after merging master, but unfortunately radio buttons still have issues. I have a couple more ideas to try though:
|
|
Thanks for linking that thread @TristanBurchett .
I'm confused by the above comment, does this mean the fix was removed from wxWidgets? |
Link to issue number:
#16683
This is a p2 issue
Summary of the issue:
Some people with low vision can't read dark text on a light backgroundCurrently,
NVDA doesn't support dark mode, which makes NVDA not usable for these users.
Description of user facing changes
Add support for dark mode in NVDA. This is configurable via "Preferences > Settings > Vision > Color theme".
The default value for this setting is Auto, which matches the system dark mode setting.
Users can also select Light or Dark to force a theme.
In addition, users can choose whether to allow use of undocumented windows APIs,
which is necessary to make the popup context menu dark.
Description of development approach
Added a new "colorTheme" setting in the "general" section. Default value is Auto.
Made this configurable in the Vision panel of the Settings dialog.
Added gui/darkMode.py which contains all the detailed logic. It exposes two functions,
initialize() and handleEvent(). handleEvent() checks if the system is in light or dark mode
and changes the foreground and background color of a particular widget and all of its
children.
Added a FilterEvent function in the toplevel App which automatically calls
handleEvent() whenever a window is created or shown.
Redesigned the Add/Edit Dictionary Entry dialogs, because they used RadioBox
which renders very poorly in dark mode (black text on dark grey background).
I changed them to use Choice dropdown instead.
Ideally this would all be handled by wxPython, but wxPython doesn't support
dark mode (wxWidgets/Phoenix#2413).
The non-python version of wx does have a function MSWEnableDarkMode(),
however even that function has to rely on undocumented APIs. If and when
wxPython supports dark mode properly, it should be fairly easy to rip out this code,
since it isn't very invasive.
Testing strategy:
Manually examined every dialog I could possibly find.
Verified that changing the color theme and then pushing the Apply button
causes the settings dialog color theme to change immediately.
Verified behavior with and without undocumented windows APIs.
Note that changing this setting has no effect until NVDA is restarted.
Known issues with pull request:
MessageBox'es are not themed. An example is the NVDA About dialog.
These dialogs are extremely modal, and there is no way to gain control
until after the user dismisses the message box.
Menu bars are not themed. An example can be seen in the Debug Log. Supporting themed
menu bars would require intercepting several undocumented events and drawing the menu items
ourselves. An example implementation is described in
https://github.com/adzm/win32-custom-menubar-aero-theme
Column titles are not themed. An example can be seen in the Dictionary dialogs.
This is implemented by the wx.ListCtrl class. The C++ implementation of
wxListCtrl::OnPaint hardcodes penColour, and there is no way to override it.
See https://github.com/wxWidgets/wxWidgets/blob/master/src/msw/listctrl.cpp
Tab controls are not themed. An example can be seen at the top of the Add-In Store.
This is implemented by the wx.Notebook class. I have not been able to figure out how
to influence the colors it uses.
BrowseMode ElementListDialog still uses a RadioBox widget, which will render
poorly in dark mode (black text on dark grey). I did not change this to use a Choice
instead, because each of the radio buttons has an accelerator key defined, and Choice
widgets do not support accelerators. Breaking the keyboard accelerators seemed
worse than making it render properly in dark mode. People who rely on this dialog
might need to set the NVDA color theme to Light if their system theme is set to dark.
Code Review Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation