High Contrast Fixes #10719
High Contrast Fixes #10719
Conversation
…utlines for toolbox categories.
| @@ -1275,8 +1270,8 @@ export class ProjectView | |||
| return previousEditor ? previousEditor.unloadFileAsync() : Promise.resolve(); | |||
| }) | |||
| .then(() => { | |||
| let hc = this.getData<boolean>(auth.HIGHCONTRAST) | |||
| return this.editor.loadFileAsync(this.editorFile, hc) | |||
| let hc = this.themeManager.isHighContrast(this.themeManager.getCurrentColorTheme()?.id); | |||
There was a problem hiding this comment.
Why aren't these instances using isCurrentThemeHighContrast?
There was a problem hiding this comment.
Just because we already had the theme manager in this file, this is slightly more efficient (if uglier)
| @@ -3892,7 +3887,7 @@ export class ProjectView | |||
| return (!!debugging != simulator.driver.isDebug()) || (!!tracing != simulator.driver.isTracing()) | |||
| } | |||
|
|
|||
| stopSimulator(unload?: boolean, opts?: pxt.editor.SimulatorStartOptions) { | |||
| stopSimulator(unload?: boolean, opts?: pxt.editor.SimulatorStartOptions): Promise<void> { | |||
There was a problem hiding this comment.
If we change this to a promise, do we need to await this being called in other areas? I see you changed it in onThemeChanged, but are the other places it's called okay with not await-ing it?
There was a problem hiding this comment.
It should be okay in other areas. I only added the return because of the specific off-then-on again scenario, where I need to actually wait until it is for-sure done setting sim state to "off" before trying to turn it on again. TBH I don't know how it was working without this, but I was able to see the race fairly clearly in my debugging.
In other places, they weren't waiting for that state to finish updating before this change, and they still aren't waiting for it now, so it shouldn't result in any change in behavior (and I didn't want to expand the scope of the change by trying to update them)
webapp/src/container.tsx
Outdated
| @@ -322,7 +323,7 @@ export class SettingsMenu extends data.Component<SettingsMenuProps, SettingsMenu | |||
|
|
|||
| renderCore() { | |||
| const hasIdentity = pxt.auth.hasIdentity(); | |||
| const highContrast = this.getData<boolean>(auth.HIGHCONTRAST) | |||
| const highContrast = ThemeManager.isCurrentThemeHighContrast(document); | |||
There was a problem hiding this comment.
Why does the container need the document passed in while the other instances are okay with the default document?
There was a problem hiding this comment.
Oh, it actually isn't necessary. This is the same as the default. I can remove it.
| @@ -632,6 +632,7 @@ export class Toolbox extends data.Component<ToolboxProps, ToolboxState> { | |||
| } | |||
| <div className="blocklyTreeRoot"> | |||
| <div | |||
| className="blocklyTreeInner" | |||
There was a problem hiding this comment.
I see a lot of changes in here for the keyboard nav stuff, will adding this classname back in possibly mess with that?
There was a problem hiding this comment.
Shouldn't cause any problems, since it's a new class and only being used in the one place I added it.
This primarily updates areas in pxt where we were checking the old true/false high contrast setting (which is no longer used) instead of the using the new theme manager. It was just an oversight in the theming work. I've created a little static helper function in the theme manager to save some repetitive lines of code.
Separately, it fixes a few places where we were seeing superfluous yellow outlines in hc mode. We have some generic highlighting css logic which, in general, is convenient. But it does seem to be causing some issues too, highlighting things we don't want highlighted. I've special-cased those out for now, but if it becomes a more common occurrence, we may need to rework it more completely.
This fixes one of the issues brought up in microsoft/pxt-microbit#6366 (inconsistent settings), but it does not remove the white background (which was by design). We can decide what we want to do for that separately and follow up in a different change, if needed.