-
Notifications
You must be signed in to change notification settings - Fork 237
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.
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
[SuperEditor] Make selected text color strategy consider each colored span (Resolves #2093) #2122
base: main
Are you sure you want to change the base?
Conversation
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would "allows overriding each color within the selected text" be more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about "can choose new selected text color based on the original text color"?
|
||
final componentTextColor = viewModel.textStyleBuilder({}).color; | ||
if (componentTextColor != null) { | ||
// Compute the selected text color for the default color of the node. If there is any |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about "can choose new selected text color based on the original text color"?
|
||
testWidgetsOnArbitraryDesktop("overrides each colored span", (tester) async { | ||
final stylesheet = defaultStylesheet.copyWith( | ||
selectedTextColorStrategy: ({required Color originalTextColor, required Color selectionHighlightColor}) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@angelosilvestre do you need anything from me to continue this PR? |
405576f
to
35ed19d
Compare
@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. |
[SuperEditor] Make selected text color strategy consider each colored span. Resolves #2093
SingleColumnLayoutComponentViewModel
invokes theSelectedTextColorStrategy
using only the default text color for the node as theoriginalColor
argument, ignoring anyColorAttribution
s. If we return the same original color, we loose the color attribution:This PR changes
SingleColumnLayoutComponentViewModel
to consider each colored span and callSelectedTextColorStrategy
for each of then: