iOS caret color in dark mode not as expected (barely visible)

[simonbengtsson]

Package Version "super_editor, GitHub, stable branch"

To Reproduce

1. Run example editor with iOS in darkmode
2. Toggle to dark theme with FAB button

Actual behavior The caret is black and hard to see since editor background is a dark grey.

Expected behavior The caret to be visible. I expect the caret color to be red since this is set in the caretStyle and what is happening on macOS.

Platform iOS. The issue is not happening on mac or web (or it does but the caret gets the correct color after refocusing the editor). Not tried other platforms.

Flutter version Flutter 3.22.0 • channel stable Framework • revision 5dcb86f68f (2 weeks ago) • 2024-05-09 07:39:20 -0500 Engine • revision f6344b75dc Tools • Dart 3.4.0 • DevTools 2.34.3

Screenshots

This is the same problem as described in https://github.com/superlistapp/super_editor/issues/1347, but I can reproduce every time on iOS.

[angelosilvestre]

@simonbengtsson To update the caret color on iOS, you need to provide a handleColor to SuperEditorIosControlsController.

That way, you can customize the caret for iOS:

[simonbengtsson]

Got it! So SuperEditorIosControlsController.handleColor on iOS, SuperEditorAndroidControlsController.controlsColor on android and DefaultCaretOverlayBuilder.caretStyle.color on all other platforms.

Since SuperEditorIosControlsController.handleColor and SuperEditorAndroidControlsController.controlsColor are final I assume then that the idea is that you create new controllers and dispose the old one if the theme changes?

[angelosilvestre]

Since SuperEditorIosControlsController.handleColor and SuperEditorAndroidControlsController.controlsColor are final I assume then that the idea is that you create new controllers and dispose the old one if the theme changes?

@simonbengtsson Yeah, you'll need to do that.

[matthew-carroll]

@simonbengtsson did that work for you? If so, can you paste the code that you had to write to get the effect you want? I'd like for us to take a look at how much work that is and see if it makes sense to create an easier pathway.

[simonbengtsson]

Actually no. When reassigning the _iosControlsController in the build method it crashes with the exception below. It feels wrong to reassign a controller in the build method so make sense I guess. Have no immediate idea for where else to do it however. For now we have set the cursor color to always be our primary color which kind of works in both dark and light mode.

OverlayPortalController.show() should not be called during build.
'package:flutter/src/widgets/overlay.dart':
Failed assertion: line 1791 pos 7: 'SchedulerBinding.instance.schedulerPhase != SchedulerPhase.persistentCallbacks'

My attempt was just adding the following lines to the _buildEditor method in example_editor.dart L370.

  Widget _buildEditor(BuildContext context) {
    final isLight = Theme.of(context).brightness == Brightness.light;

    // Added code
    _iosControlsController.dispose();
    _iosControlsController = SuperEditorIosControlsController(
      handleColor: isLight ? Colors.black : Colors.redAccent,
    );

    [...]

[angelosilvestre]

@simonbengtsson You should try to update it on the didChangeDependencies method.

[simonbengtsson]

Tried now, but didChangeDependencies was not called when "Change theme" button in the example editor is clicked. Used the following code:

  @override
  void didChangeDependencies() {
    super.didChangeDependencies();
    final isLight = Theme.of(context).brightness == Brightness.light;
    _iosControlsController.dispose();
    _iosControlsController = SuperEditorIosControlsController(
      handleColor: isLight ? Colors.black : Colors.redAccent,
    );
    print('Reassigned ${isLight}'); // Only called once, not on theme change
  }

[angelosilvestre]

@simonbengtsson @matthew-carroll I took a look at this. The didChangeDependencies isn't been called because used there isn't the themed context. The ExampleEditor's ancestor theme doesn't change from light to dark, only the theme that is inside ExampleEditor, around the editor, that changes.

To solve this, the theme should be moved up, outside of ExampleEditor.

@override
Widget build(BuildContext context) {
  return ValueListenableBuilder(
    valueListenable: _brightness,
    builder: (context, brightness, child) {
      return Theme(
        data: ThemeData(brightness: brightness),
        child: child!,
      );
    },
    child: Builder(
      // This builder captures the new theme
      builder: (themedContext) {
      ...
     }   
   ),
 );
}

[matthew-carroll]

@angelosilvestre if that resolves the issue in the demo app, let's go ahead and make the change. Then let's take another look at how much code is involved with such a simple behavior.

[angelosilvestre]

@matthew-carroll After taking a better look, I noticed that SuperEditorIosHandlesDocumentLayerBuilder has a handleColor property, so we shouldn't need to rely on SuperEditorIosControlsController for that.

However, I noticed this isn't working correctly even for desktop. The color doesn't update immediately:

https://github.com/superlistapp/super_editor/assets/7597082/d77e432a-331c-4d06-a24c-068b546abcd8

This looks like a deeper problem in ContentLayers. Maybe it isn't working correctly when changing the layer builders?

[matthew-carroll]

@angelosilvestre can you dig deeper and try to find out where the problem is?

[angelosilvestre]

@matthew-carroll After taking a deeper look, I found that the cause is that performLayout isn't being called in ContentLayers. As a result, the layer builders aren't re-creating the layers.

When switching between light and dark mode, the only thing that we change in the editor is the font color. Because of that, only markNeedsPaint gets called, and not markNeedsLayout. We can confirm this by also changing the font size in the dark mode styles. If the font size changes, the layout is invalidated and the layer builder recreates the layer.

A possible solution can be achieved by making every SuperEditorLayerBuilder implement the == and then on SuperEditorState.didUpdateWidget checking if any layer builder changed. If a layer builder changed, we should call markNeedsLayout on the ContentLayers render object.

What do you think of that approach? Do you have any other ideas?

[matthew-carroll]

@angelosilvestre I'm not entirely sure about the implications here, but on the surface of the issue, I don't know why we'd want to re-run layout for color changes. Layout is generally expensive and we should only run it when layout truly changes.

Is there a particular reason that you're suggesting we re-run layout for color changes?

[angelosilvestre]

Because without a re-run of the layout, the overlayers won't be rebuilt, so they won't pick the new colors.

[matthew-carroll]

To help me understand exactly what's happening here, can you please write a failing test and open a PR with that test failure?

[angelosilvestre]

@matthew-carroll Opened https://github.com/superlistapp/super_editor/pull/2388

[lucasjinreal]

I set handlerColor when in dark mode the cursor color still black....

[angelosilvestre]

@simonbengtsson @lucasjinreal This should now work as expected. Please see this comment https://github.com/superlistapp/super_editor/issues/2302#issuecomment-2566422439

[lucasjinreal]

Hi, can u point out which version start from could fix this?

[angelosilvestre]

@lucasjinreal We didn't release a new pub version yet, but you can add a dependency to the latest main.

[angelosilvestre]

@matthew-carroll I believe we can close this issue. It was already fixed and we have new releases that include the fix.