superlistapp / super_editor

A Flutter toolkit for building document editors and readers
https://superlist.com/SuperEditor/
MIT License
1.59k stars 230 forks source link

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

Open simonbengtsson opened 1 month ago

simonbengtsson commented 1 month ago

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 issue

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 commented 1 month ago

@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 commented 4 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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

simonbengtsson commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

@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 commented 1 week ago

@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 commented 1 week ago

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