[SuperEditor] Make selected text color strategy consider each colored span (Resolves #2093) #2122

angelosilvestre · 2024-06-22T19:58:22Z

[SuperEditor] Make selected text color strategy consider each colored span. Resolves #2093

SingleColumnLayoutComponentViewModel invokes the SelectedTextColorStrategy using only the default text color for the node as the originalColor argument, ignoring any ColorAttributions. If we return the same original color, we loose the color attribution:

This PR changes SingleColumnLayoutComponentViewModel to consider each colored span and call SelectedTextColorStrategy for each of then:

matthew-carroll · 2024-07-01T05:26:33Z

super_editor/test/super_editor/supereditor_selection_test.dart

@@ -88,6 +88,79 @@ void main() {
        expect(richText.getSpanForPosition(const TextPosition(offset: 5))!.style!.color, Colors.green);
        expect(richText.getSpanForPosition(const TextPosition(offset: 16))!.style!.color, Colors.green);
      });
+
+      testWidgetsOnArbitraryDesktop("overrides each colored span", (tester) async {


When I read the name of this test, I don't know what "overrides each colored span" means. I don't know the UX impetus for this test, nor what the UI result is expected to be.

Moreover, when I look down at the bottom of the test to see what the expectations are looking for, the comment above them simply says that all the colors have changed. Whatever this test is verifying, it's currently too difficult to figure out what that is.

Would "allows overriding each color within the selected text" be more clear?

how about "can choose new selected text color based on the original text color"?

matthew-carroll · 2024-07-01T05:31:46Z

super_editor/lib/src/default_editor/layout_single_column/_styler_user_selection.dart

+
+          final componentTextColor = viewModel.textStyleBuilder({}).color;
+          if (componentTextColor != null) {
+            // Compute the selected text color for the default color of the node. If there is any


To help avoid ambiguity with the concept of "selection color", any place where you're currently saying "selected text color", you should probably say "color of the selected text".

Also, I think this comment is overall difficult to follow. I think the following is what you're doing:

// Copy the existing text, switching the color of the text from the regular text color to the // color of selected text. For example, text that's normally black, might become white when // selected with a dark selection box color. // // Any text with an explicit color attribution will remain unaffected, e.g., a red word will // remain red.

// Any text with an explicit color attribution will remain unaffected, e.g., a red word will
// remain red.

This isn't the case. For each span with a different color, we call _selectedTextColorStrategy with that color as the originalTextColor. If the method returns a different color, the returned color will override the existing color.

matthew-carroll · 2024-07-01T05:33:26Z

super_editor/lib/src/default_editor/layout_single_column/_styler_user_selection.dart

+            // text color attributions in the selected range, they will override this color.
+            textWithSelectionAttributions = viewModel.text.copyText(0)
+              ..addAttribution(
+                ColorAttribution(_selectedTextColorStrategy!(


I'm not sure the overall assumptions of this change are correct. We're assuming that any text with a color applied to it should retain that same color when selected.

That might be what some people want, but it's probably not true in general, for the same reason that we're letting people change the color of selected text in the first place. Colors that are high contrast when unselected might be low contrast when selected. This applies to spans of text with color attributions, too.

Shouldn't the selected text color strategy get an opportunity to switch any given color, as desired?

Indeed, as a user, I would like to know the original color (after the attributions were applied), but I want the color returned by selectedTextColorStrategy to be displayed without further modifications. I just need to know about the original color so that I can calculate the correct color to be displayed.

I'm not sure the overall assumptions of this change are correct. We're assuming that any text with a color applied to it should retain that same color when selected.

As explained in this comment, the selected color strategy has to opportunity to switch any given color. The difference now is that, if the selected range has multiple different colored spans, the selected color strategy can return a new color for each existing color.

@angelosilvestre can you add a test for that so I can see it in action?

@matthew-carroll The test referenced in this comment https://github.com/superlistapp/super_editor/pull/2122/files#r1660478509 is an example

matthew-carroll · 2024-07-09T00:56:17Z

super_editor/test/super_editor/supereditor_selection_test.dart

@@ -88,6 +88,79 @@ void main() {
        expect(richText.getSpanForPosition(const TextPosition(offset: 5))!.style!.color, Colors.green);
        expect(richText.getSpanForPosition(const TextPosition(offset: 16))!.style!.color, Colors.green);
      });
+
+      testWidgetsOnArbitraryDesktop("overrides each colored span", (tester) async {


how about "can choose new selected text color based on the original text color"?

matthew-carroll · 2024-07-09T00:58:10Z

super_editor/test/super_editor/supereditor_selection_test.dart

+
+      testWidgetsOnArbitraryDesktop("overrides each colored span", (tester) async {
+        final stylesheet = defaultStylesheet.copyWith(
+          selectedTextColorStrategy: ({required Color originalTextColor, required Color selectionHighlightColor}) {


If we consider that editors will be used in a light mode and a dark mode, it seems likely that users would benefit from a strategy that works well in dark mode, to go with our existing strategy for light mode. A dark mode strategy should probably use a heuristic that inverts the luminance of each existing text color. Can you see if you can put together a reasonable dark mode strategy that automatically selects new colors and then we could test that actual strategy here?

I tried a naive solution inverting the lightness of a HLS color with the following code, but it doesn't look great:

Color darkModeSelectedColorStrategy({ required Color originalTextColor, required Color selectionHighlightColor, }) { final hslColor = HSLColor.fromColor(originalTextColor); return hslColor.withLightness(1.0 - hslColor.lightness).toColor(); }

Not selected:

With a strategy that returns the original color:

Color with an inverse lightness.

What do you think? Do you have other ideas ? Maybe we should take the selectionHighlightColor also in consideration.

Maybe we should take the selectionHighlightColor also in consideration.

Yeah I think we probably should. The primary consideration is to ensure that the selected text contrasts with the selection. Do you know of any other app references to look at for this, where selection colors are different from typical browser selections?

Word (at least on Mac) seems to change the text color slightly if the selection color is the exact same as the text color.

I tried some apps:

Notion doesn't seem to change the font color when the text is selected:

Apple notes changes all text to white:

Google docs don't change the font color:

Word online also doesn't seem to change the font color:

matthew-carroll · 2024-08-10T19:45:48Z

@angelosilvestre do you need anything from me to continue this PR?

… span (Resolves #2093)

angelosilvestre · 2024-08-18T18:54:47Z

@matthew-carroll Updated the example app to use the same behavior as Apple Notes to color the text in dark mode.

Also, update a test name.

matthew-carroll · 2024-09-14T17:31:18Z

super_editor/example/lib/demos/example_editor/example_editor.dart

+  /// Makes text white, for use during dark mode styling.
+  ///
+  /// This is the same behavior observed in Apple Notes.
+  Color _darkModeSelectedColorStrategy({


Did we say that we wanted Apple Notes style highlighting as the default in Super Editor? If so, why is this in the example app? Shouldn't we have a public default object in Super Editor that does this in dark mode, and does the other thing in light mode?

Are you saying that we should include this in the default stylesheet? If so, we would need to include the BuildContext in the SelectedTextColorStrategy signature, otherwise we won't know if we are in light or dark mode.

angelosilvestre requested a review from matthew-carroll June 22, 2024 19:58

matthew-carroll reviewed Jul 1, 2024

View reviewed changes

matthew-carroll reviewed Jul 9, 2024

View reviewed changes

angelosilvestre added 2 commits August 18, 2024 15:33

[SuperEditor] Make selected text color strategy consider each colored…

7c2fbc9

… span (Resolves #2093)

Make the text white while in dark mode

35ed19d

angelosilvestre force-pushed the 2093_selected-color-attribution branch from 405576f to 35ed19d Compare August 18, 2024 18:52

Update test name

cde650e

angelosilvestre requested a review from matthew-carroll August 18, 2024 18:54

matthew-carroll reviewed Sep 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SuperEditor] Make selected text color strategy consider each colored span (Resolves #2093) #2122

[SuperEditor] Make selected text color strategy consider each colored span (Resolves #2093) #2122

angelosilvestre commented Jun 22, 2024

matthew-carroll Jul 1, 2024

angelosilvestre Jul 3, 2024

matthew-carroll Jul 9, 2024

matthew-carroll Jul 1, 2024

angelosilvestre Jul 3, 2024

matthew-carroll Jul 1, 2024

KevinBrendel Jul 3, 2024

angelosilvestre Jul 3, 2024

matthew-carroll Jul 8, 2024

angelosilvestre Jul 8, 2024

matthew-carroll Jul 9, 2024

matthew-carroll Jul 9, 2024

angelosilvestre Jul 10, 2024 •

edited

Loading

matthew-carroll Jul 17, 2024

KevinBrendel Jul 17, 2024

angelosilvestre Aug 14, 2024

matthew-carroll commented Aug 10, 2024

angelosilvestre commented Aug 18, 2024

matthew-carroll Sep 14, 2024

angelosilvestre Sep 16, 2024

[SuperEditor] Make selected text color strategy consider each colored span (Resolves #2093) #2122

Are you sure you want to change the base?

[SuperEditor] Make selected text color strategy consider each colored span (Resolves #2093) #2122

Conversation

angelosilvestre commented Jun 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelosilvestre Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-carroll commented Aug 10, 2024

angelosilvestre commented Aug 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelosilvestre Jul 10, 2024 •

edited

Loading