Desktop: Remove cm-strong css color specification #5732

hieuthi · 2021-11-15T09:01:58Z

Current version assign strong tag and cm-strong element with a specific color (color: ${theme.colorBright};) which entangle the renderers' behaviors.

For the Markdown-it side it makes **==Emphasis Text==** and ==**Emphasis Text**== rendered differently.
For the CodeMirror side: it makes customization more convoluted because cm-strong does not inherit it's parent style.

Unless there is a specific reason that i don't know of, when it comes to css it is better to achieve the same effect with as less code as possible so we should trust that cm-strong will inherit the correct color from it's parent.

I didn't found the same issue with italic.

cm-strong should inherit `color` from parent

Strong should only make text bolder

laurent22 · 2021-11-15T19:31:12Z

packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/CodeMirror.tsx

@@ -498,10 +498,6 @@ function CodeMirror(props: NoteBodyEditorProps, ref: any) {
 				padding-left: .2em;
 			}

-			div.CodeMirror span.cm-strong {
-				color: ${theme.colorBright};
-			}


If we remove this I'm not clear where it would get the styling from now?

laurent22 · 2021-11-15T19:31:52Z

packages/renderer/noteStyle.ts

@@ -49,9 +49,6 @@ export default function(theme: any, options: Options = null) {
 			padding-bottom: ${formatCssSize(theme.bodyPaddingBottom)};
 			padding-top: ${formatCssSize(theme.bodyPaddingTop)};
 		}
-		strong {
-			color: ${theme.colorBright};
-		}


Same here? We set it like this so that it takes the colour from the theme but if we remove it, then what colour would it be? Just black?

Ii's color will be situational and taken from its immediate parent. Most of the time it will take from body (color: ${theme.color};) meaning it will be black in light theme and white in dark theme and so on for other theme.

But it should not have its own color because when strong is inside some other tags like ==**Text**== we want it to have mark color. Mark change the background-color so it has the responsibility to make sure the text is readable. While strong and italic do no change background-color and so it should not change color and inherit color from its parent instead.

Hmm, I see your point, but colorBright can be different from the parent. In some theme I think it's bright green for example, while normal text is white. If the mark background color makes the text unreadable then that's where the problem is - the theme needs to be fixed so that the colorBright and mark color work properly together.

Sure you can temporary fix this issue by assigning a specific color to strong or simply add more css, but it will keep the entanglement and increase the techincal debt to downstream (plugin, users,...). The inline text formats (strong, italic, underline,...) have very specific specifications or "promises" by doing more than they should it unnecessary confuses end-user. To support my case i link to some closed and open issues in which the bold color is "off" or users wonder why strong text has different color than normal color.

#3012 (comment)

#2921 (comment)

#5681 (comment)

I understand that some may want bold text to have bright color to emphasis it more but that should be done by end-user not default behaviors. This issue will keep appearing again and again if not addressed.

That being said, it is not impossible to fix it downstream for now so feel free to think about it and decide it yourself.

Hmm, yes I kind of agree with you, but then it's bigger issue: we'd need to get rid of the colorBright property entirely and of course making sure we're not breaking any UI, editor or theme doing so.

We don't have to bundle <strong> and colorBright problem together, at least for now. A search in code base https://github.com/laurent22/joplin/search?p=1&q=colorBright shows that colorBright is only used in two (technically three) places:

Strong element for CodeMirror and Markdown-it

And GotoAnything plugin.

But even if there is not explicitly use of colorBright in the core codebase we can keep colorBright as a convenient/reliable variable for downstream (plugin, users). Remove it now can cause significant changes for end-user as we don't know how they are using it.

You can open the discussion about remove colorBright again when significant changes in GUI/theme structure is undergoing. For now it's best to keep this PR atomic and just disentangle <strong> from colorBright which is unlikely will break anything.

Yes but if we merge this PR, colorBright is almost not used at all anywhere, which is why it should be removed entirely. If it's used in a plugin so be it, I don't think it would be a major breaking change.

I can remove colorBright completely from the code base if you're fine with it. Although I worry it will break someone's setup and they will create an issue to complain about it.

hieuthi · 2021-11-30T11:44:25Z

I completely remove colorBright from the desktop app. This change affect two components not just one:

It make sure cm-strong and strong has the same color as it parent
It change the color of some elements of GotoAnything from $theme.colorBright to $theme.color which can look a bit dull depend on theme.

I initially create this PR just for the first component but now it changes two (and quite a lot of files) so I log it here for future reference.

laurent22 · 2021-12-05T19:20:18Z

It change the color of some elements of GotoAnything from $theme.colorBright to $theme.color which can look a bit dull depend on theme.

Could you try to add text-decoration: underline to see if it's any better?

laurent22 · 2021-12-05T23:10:34Z

Also there's a falling test https://github.com/laurent22/joplin/runs/4423691010?check_suite_focus=true#step:9:545

hieuthi · 2021-12-08T13:15:20Z

I fixed the failing test and mark the search fragments (with color: ${theme.searchMarkerColor}; background-color: ${theme.searchMarkerBackgroundColor}) instead of bold them. This will give it a somewhat uniform appearance with search function and make sure the text standout.

laurent22 · 2021-12-28T09:56:21Z

This will give it a somewhat uniform appearance with search function and make sure the text standout.

Indeed that makes sense. Thanks for the update.

hieuthi added 2 commits November 15, 2021 17:37

Remove cm-strong color css

86ec928

cm-strong should inherit `color` from parent

Remove css color from strong tag

4fea275

Strong should only make text bolder

laurent22 reviewed Nov 15, 2021

View reviewed changes

tessus added the desktop All desktop platforms label Nov 19, 2021

hieuthi added 3 commits November 29, 2021 20:12

Remove mention of bold in comment

12f6387

Change color used by GotoAnything from colorBright to color

ab133dd

Remove colorBright from all themes

9a67058

hieuthi added 2 commits December 8, 2021 22:08

Fixed failed test

197a929

Mark search fragments instead of bold them

8438d2e

laurent22 merged commit 46438a5 into laurent22:dev Dec 28, 2021

hieuthi deleted the fix_cm_strong_color branch December 29, 2021 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop: Remove cm-strong css color specification #5732

Desktop: Remove cm-strong css color specification #5732

hieuthi commented Nov 15, 2021

laurent22 Nov 15, 2021

laurent22 Nov 15, 2021

hieuthi Nov 15, 2021

laurent22 Nov 17, 2021

hieuthi Nov 17, 2021

laurent22 Nov 28, 2021

hieuthi Nov 29, 2021

laurent22 Nov 29, 2021

hieuthi Nov 29, 2021

hieuthi commented Nov 30, 2021 •

edited

Loading

laurent22 commented Dec 5, 2021

laurent22 commented Dec 5, 2021 •

edited

Loading

hieuthi commented Dec 8, 2021

laurent22 commented Dec 28, 2021

Desktop: Remove cm-strong css color specification #5732

Desktop: Remove cm-strong css color specification #5732

Conversation

hieuthi commented Nov 15, 2021

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

hieuthi commented Nov 30, 2021 • edited Loading

laurent22 commented Dec 5, 2021

laurent22 commented Dec 5, 2021 • edited Loading

hieuthi commented Dec 8, 2021

laurent22 commented Dec 28, 2021

hieuthi commented Nov 30, 2021 •

edited

Loading

laurent22 commented Dec 5, 2021 •

edited

Loading