Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7c2fbc9
[SuperEditor] Make selected text color strategy consider each colored…
angelosilvestre Jun 22, 2024
35ed19d
Make the text white while in dark mode
angelosilvestre Aug 18, 2024
cde650e
Update test name
angelosilvestre Aug 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions super_editor/example/lib/demos/example_editor/example_editor.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,16 @@ class _ExampleEditorState extends State<ExampleEditor> {
_editorFocusNode.requestFocus();
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

required Color originalTextColor,
required Color selectionHighlightColor,
}) {
return Colors.white;
}

@override
Widget build(BuildContext context) {
return ValueListenableBuilder(
Expand Down Expand Up @@ -397,6 +407,7 @@ class _ExampleEditorState extends State<ExampleEditor> {
selectionColor: Colors.red.withOpacity(0.3),
),
stylesheet: defaultStylesheet.copyWith(
selectedTextColorStrategy: isLight ? null : _darkModeSelectedColorStrategy,
addRulesAfter: [
if (!isLight) ..._darkModeStyles,
taskStyles,
Expand Down
61 changes: 44 additions & 17 deletions super_editor/lib/src/default_editor/layout_single_column/_styler_user_selection.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,25 +132,52 @@ class SingleColumnLayoutSelectionStyler extends SingleColumnLayoutStylePhase {
editorStyleLog.finer(' - extent: ${textSelection?.extent}');

if (viewModel is TextComponentViewModel) {
final componentTextColor = viewModel.textStyleBuilder({}).color;

final textWithSelectionAttributions =
textSelection != null && _selectedTextColorStrategy != null && componentTextColor != null
? (viewModel.text.copyText(0)
..addAttribution(
ColorAttribution(_selectedTextColorStrategy!(
originalTextColor: componentTextColor,
selectionHighlightColor: _selectionStyles.selectionColor,
)),
SpanRange(textSelection.start, textSelection.end - 1),
// The selected range might already have a color attribution. We want to override it
// with the selected text color.
overwriteConflictingSpans: true,
))
: viewModel.text;
AttributedText? textWithSelectionAttributions;

if (textSelection != null && _selectedTextColorStrategy != null) {
final selectedRange = SpanRange(textSelection.start, textSelection.end - 1);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// 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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

originalTextColor: componentTextColor,
selectionHighlightColor: _selectionStyles.selectionColor,
)),
selectedRange,
// Override any existing color attributions.
overwriteConflictingSpans: true,
);
}

final coloredSpans = viewModel.text.getAttributionSpansInRange(
attributionFilter: (attr) => attr is ColorAttribution,
range: selectedRange,
// We should only change the selected portion of each span.
resizeSpansToFitInRange: true,
);
if (coloredSpans.isNotEmpty) {
// Compute and apply the selected text color for each span that has a color attribution.
textWithSelectionAttributions ??= viewModel.text.copyText(0);
for (final span in coloredSpans) {
textWithSelectionAttributions.addAttribution(
ColorAttribution(_selectedTextColorStrategy!(
originalTextColor: (span.attribution as ColorAttribution).color,
selectionHighlightColor: _selectionStyles.selectionColor,
)),
SpanRange(span.start, span.end),
// Override any existing color attributions.
overwriteConflictingSpans: true,
);
}
}
}

viewModel
..text = textWithSelectionAttributions
..text = textWithSelectionAttributions ?? viewModel.text
..selection = textSelection
..selectionColor = _selectionStyles.selectionColor
..highlightWhenEmpty = highlightWhenEmpty;
Expand Down
74 changes: 74 additions & 0 deletions super_editor/test/super_editor/supereditor_selection_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,80 @@ 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("can choose new selected text color based on the original text color",
(tester) async {
final stylesheet = defaultStylesheet.copyWith(
selectedTextColorStrategy: ({required Color originalTextColor, required Color selectionHighlightColor}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@angelosilvestre angelosilvestre Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

SCR-20240709-tcza

With a strategy that returns the original color:

SCR-20240709-tdil

Color with an inverse lightness.

SCR-20240709-tdpn

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried some apps:

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

image

image

Apple notes changes all text to white:

image

image

Google docs don't change the font color:

image

image

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

image

image

if (originalTextColor == Colors.green) {
return Colors.red;
}

if (originalTextColor == Colors.yellow) {
return Colors.blue;
}

return Colors.white;
},
);

// Pump an editor with a paragraph with the following colors:
// Lorem ipsum dolor
// gggggg-----------
// ------yyyyyy-----
// ------------bbbbb (black, the default color)
await tester //
.createDocument()
.withCustomContent(
MutableDocument(
nodes: [
ParagraphNode(
id: '1',
text: AttributedText(
'Lorem ipsum dolor',
AttributedSpans(
attributions: [
SpanMarker(
attribution: const ColorAttribution(Colors.green),
offset: 0,
markerType: SpanMarkerType.start),
SpanMarker(
attribution: const ColorAttribution(Colors.green),
offset: 5,
markerType: SpanMarkerType.end),
SpanMarker(
attribution: const ColorAttribution(Colors.yellow),
offset: 6,
markerType: SpanMarkerType.start),
SpanMarker(
attribution: const ColorAttribution(Colors.yellow),
offset: 11,
markerType: SpanMarkerType.end),
],
),
),
),
],
),
)
.useStylesheet(stylesheet)
.pump();

// Triple tap to select the whole paragraph.
await tester.tripleTapInParagraph('1', 2);

// Ensure that all spans changed colors.
final richText = SuperEditorInspector.findRichTextInParagraph('1');

expect(richText.getSpanForPosition(const TextPosition(offset: 0))!.style!.color, Colors.red);
expect(richText.getSpanForPosition(const TextPosition(offset: 5))!.style!.color, Colors.red);

expect(richText.getSpanForPosition(const TextPosition(offset: 6))!.style!.color, Colors.blue);
expect(richText.getSpanForPosition(const TextPosition(offset: 11))!.style!.color, Colors.blue);

expect(richText.getSpanForPosition(const TextPosition(offset: 12))!.style!.color, Colors.white);
expect(richText.getSpanForPosition(const TextPosition(offset: 16))!.style!.color, Colors.white);
});
});

testWidgetsOnArbitraryDesktop("calculates upstream document selection within a single node", (tester) async {
Expand Down
Loading