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"?

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.

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

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

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: