Bug 137705 - Stylist not using document colors making it hard to read fonts not using automatic color
Summary: Stylist not using document colors making it hard to read fonts not using auto...
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
7.0.2.2 release
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: KDE Sidebar-Styles Styles-Preview Linux-Dark-Mode
  Show dependency treegraph
 
Reported: 2020-10-23 12:17 UTC by medmedin2014
Modified: 2022-06-29 07:10 UTC (History)
7 users (show)

See Also:
Crash report or crash signature:
Regression By:


Attachments
Setting style color to black makes hard to read style name (307.71 KB, image/png)
2020-10-23 12:17 UTC, medmedin2014
Details

Note You need to log in before you can comment on or make changes to this bug.
Description medmedin2014 2020-10-23 12:17:05 UTC
Created attachment 166661 [details]
Setting style color to black makes hard to read style name

When I use dark breeze theme and change color of the style from automatic to black then the name of the style in styles sidebar is displayed also with black foreground which make it hard to see it. See attached image for more info.
If it's necessary for the style to show the modified foreground color it should also be displayed with current background color white to make it visible.
Comment 1 medmedin2014 2020-10-23 12:18:52 UTC
Version: 7.0.2.2
Build ID: 8349ace3c3162073abd90d81fd06dcfb6b36b994
CPU threads: 2; OS: Linux 5.4; UI render: default; VCL: kf5
Locale: en-US (en_US.UTF-8); UI: en-US
Calc: threaded

Operating System: Manjaro Linux
KDE Plasma Version: 5.19.5
KDE Frameworks Version: 5.74.0
Qt Version: 5.15.1
Kernel Version: 5.4.72-1-MANJARO
OS Type: 64-bit
Comment 2 Heiko Tietze 2020-10-26 12:57:10 UTC
This issue is similar to bug 115507 but different in regards that we ignore the page style and use the application color for list/tree background (unless the paragraph background is set). So if the "Show previews" checkbox is checked we should color the background according the current page style.

Code pointer:

sfx2/source/dialog/templdlg.cxx > PreviewHdl
mxFmtLb|mxTreeBox->set_background();

(Don't know how to read the page style attributes. Mike, can you help out please?)
Comment 3 Mike Kaganski 2020-10-26 13:02:46 UTC
Using a UI color (changing with UI dark theme) with something like style preview, which would *sometimes* use unrelated "current" page style (why?) seems wrong.

Instead, just make the style preview always use white background, unless set explicitly.
Comment 4 Heiko Tietze 2020-10-26 13:08:16 UTC
(In reply to Mike Kaganski from comment #3)
> Instead, just make the style preview always use white background, unless set
> explicitly.

Was my first idea too (and pretty easy to realize) but would be incorrect. Users may have, for example, a black page background and white text. And we rejected bug 115507 mostly because the stylist is supposed to be WYSIWYG.
Comment 5 Mike Kaganski 2020-10-26 13:13:37 UTC
(In reply to Heiko Tietze from comment #4)
> Was my first idea too (and pretty easy to realize) but would be incorrect.
> Users may have, for example, a black page background and white text. And we
> rejected bug 115507 mostly because the stylist is supposed to be WYSIWYG.

It is *not* incorrect. Page background is absolutely orthogonal to e.g. paragraph style preview. Paragraph gets its background from anything it happens to be on: from table cell; from frame; from section, from header settings ... page background is just one possibility. So using *paragraph* background when set, and falling back to *white* consistently is correct; using a randomly chosen one of possible background sources is wrong.
Comment 6 Mike Kaganski 2020-10-26 13:15:46 UTC
(In reply to Mike Kaganski from comment #5)

(and which of multiple page styles should be used here?)
Comment 7 Heiko Tietze 2020-10-26 13:18:46 UTC
(In reply to Mike Kaganski from comment #6)
> (and which of multiple page styles should be used here?)

The currently active - and this is the only valid argument against using the PgS. White is just one color, common but not always applicable. But as a compromise we could go with Application Color > Document Background- which also solves the trouble of using the current PgS.
Comment 8 Mike Kaganski 2020-10-26 13:22:41 UTC
(In reply to Heiko Tietze from comment #7)
> The currently active - and this is the only valid argument against using the
> PgS.

Completely disagree with "this is the only valid argument". Then, what is "currently active"? Where the cursor is? or which is on screen when scrolling? Do you intend to add flickering when scrolling or when using arrows? ...

> White is just one color, common but not always applicable. But as a
> compromise we could go with Application Color > Document Background- which
> also solves the trouble of using the current PgS.

Well, using the Document Background as fallback instead of white is fine.
Comment 9 Heiko Tietze 2020-10-26 14:57:03 UTC
While this works I tend to disagree with the request. Setting font color to black is not the supposed workflow (and Automatic / COL_AUTO would make them appear in system default, which is white on dark themes).


This code fails when the tree/list is updated (eg. Hide) and misses the background for the tree canvas; only the rows with content are drawn.

diff --git a/sfx2/source/dialog/templdlg.cxx b/sfx2/source/dialog/templdlg.cxx
index e9bfc17fa876..6458cd31275d 100644
--- a/sfx2/source/dialog/templdlg.cxx
+++ b/sfx2/source/dialog/templdlg.cxx
@@ -33,6 +33,7 @@
 #include <svl/intitem.hxx>
 #include <svl/stritem.hxx>
 #include <svl/style.hxx>
+#include <svtools/colorcfg.hxx>
 #include <comphelper/processfactory.hxx>
 #include <comphelper/sequenceashashmap.hxx>
 #include <unotools/intlwrapper.hxx>
@@ -285,7 +286,8 @@ IMPL_LINK(SfxCommonTemplateDialog_Impl, CustomRenderHdl, weld::TreeView::render_
     if (bSelected)
         rRenderContext.SetTextColor(rStyleSettings.GetHighlightTextColor());
     else
-        rRenderContext.SetTextColor(rStyleSettings.GetDialogTextColor());
+        rRenderContext.SetTextColor(
+            svtools::ColorConfig().GetColorValue(svtools::FONTCOLOR).nColor);
 
     bool bSuccess = false;
 
@@ -773,6 +775,8 @@ void SfxCommonTemplateDialog_Impl::Initialize()
     mxFmtLb->set_visible(!bHierarchical);
     mxTreeBox->set_visible(bHierarchical);
 
+    //update colors
+    PreviewHdl(*mxPreviewCheckbox);
     Update_Impl();
 }
 
@@ -1940,9 +1944,14 @@ IMPL_LINK_NOARG(SfxCommonTemplateDialog_Impl, PreviewHdl, weld::Button&, void)
 
     mxFmtLb->clear();
     mxFmtLb->set_column_custom_renderer(0, bCustomPreview);
+    mxFmtLb->set_background(
+        bCustomPreview ? svtools::ColorConfig().GetColorValue(svtools::DOCCOLOR).nColor
+                       : Application::GetSettings().GetStyleSettings().GetWindowColor());
     mxTreeBox->clear();
     mxTreeBox->set_column_custom_renderer(0, bCustomPreview);
-
+    mxTreeBox->set_background(
+        bCustomPreview ? svtools::ColorConfig().GetColorValue(svtools::DOCCOLOR).nColor
+                       : Application::GetSettings().GetStyleSettings().GetWindowColor());
     FamilySelect(nActFamily, true);
 }
Comment 10 Mike Kaganski 2020-10-26 15:02:34 UTC
(In reply to Heiko Tietze from comment #9)
> While this works I tend to disagree with the request. Setting font color to
> black is not the supposed workflow (and Automatic / COL_AUTO would make them
> appear in system default, which is white on dark themes).

"Not the supposed workflow" is a wrong argument. There's no "single intended workflow" for most things in general-purpose software. An existing user-modifiable property (e.g., text style color) means that it is an *intended* workflow to use it. That you yourself don't use it doesn't mean others have no valid reasons doing so.

Please let's discuss the code in gerrit - I don't easily see the context here, and can't advise. Thanks for your effort!
Comment 11 Heiko Tietze 2021-02-19 09:47:32 UTC
This patch https://gerrit.libreoffice.org/c/core/+/105786 colors the background of the tree but only where nodes are and not the whole canvas. Abandoned.
Comment 12 Jan-Marek Glogowski 2021-09-10 12:44:59 UTC
Nothing in this bug is actually KDE related, so I'm removing it.

Maybe a compromise would be to test white and black background (bg) against the font color, if the style doesn't provide an explicit bg, and select the bg by the largest contrast?

Not sure if that is more confusing, but at least it should always be readable.
Comment 13 Jan-Marek Glogowski 2021-09-10 13:20:44 UTC
Ok - I re-added KDE, as it's probably easiest to repo.

An other idea that came to mind, but might not be the best solution: GIMP creates a checkered background with two gray colors, if the background is represented as transparent / not set. Maybe this could be used, if no explicit background is set? Or we introduce some other indicator (icon?)?

Then there is the general problem, what colors to use, if the entry is highlighted / selected. But maybe highlight should work like the math element selection on the left side, where it just adds a frame?

For me this whole problem should be addressed in a more generic way by the UX team with all the cases of foreground / font and background color and how style preview widgets should display them. It feels like all the bugs seem to re-iterate everything again, depending on themed document background, themed default font color, style settings, etc.
Comment 14 Heiko Tietze 2021-09-11 07:42:57 UTC
(In reply to Jan-Marek Glogowski from comment #13)
> checkered background with two gray colors, if the background is
> represented as transparent / not set.

The ticket is about the Stylist (the styles sidebar deck) and the fact that this tree is drawn with application instead of document background (white font color on black background for Breeze Dark). See also c11.
Comment 15 Jan-Marek Glogowski 2021-09-11 09:28:57 UTC
(In reply to Heiko Tietze from comment #14)
> (In reply to Jan-Marek Glogowski from comment #13)
> > checkered background with two gray colors, if the background is
> > represented as transparent / not set.
> 
> The ticket is about the Stylist (the styles sidebar deck) and the fact that
> this tree is drawn with application instead of document background (white
> font color on black background for Breeze Dark). See also c11.

I'm aware, but that is just this exact problem of this bug.

My attempt was trying to make (design) people start to think more about a generic style preview widget. What specifics of a style would one want to show in a preview and how to keep the combination readable. E.g. we already have something like this for font names, where we check that the font actually has the glyphs to display it's name, otherwise we fall back to a default and a styled example text. We can at least test contrast between foreground and background and try to "fix" stuff. But still inform the user, that "this preview is altered", because the real would result in "this", just like for fonts. And how to represent a selection in this context. So something like the font approach, but for these color problems. OTOH the notebookbar's "styles preview" widget on master already shows the preview and adds the unstyled name in a 2nd row. Option for the treeview too?

Or in addition, have something like a permanent tooltip while hovering over the potentially unreadable preview. It shows up after some time but doesn't fade out, unless your mouse leaves the widget. Includes the basic data, like name, colors and font. The sidebar tree widget already shows a tooltip with the name, while the styles preview in the notebookbar just says "styles preview". Maybe my proposed extended tooltip should be optional.
Comment 16 Jan-Marek Glogowski 2021-09-11 09:38:17 UTC
Even if we can't come up with some good general description / solution, just listing the problems one may face when implementing / changing such a widget might help people correctly testing functionality. That is generally a major problem for myself. My latest example / failure is tdf#144412, where I initially didn't even "groked" it to be a feature.
Comment 17 Heiko Tietze 2021-09-13 09:02:11 UTC
Sure, would be a long list ;-). One example is white font color on white backgrounds, or vice versa, in bug 115507. While we deal with this corner case for the dropdown, see bug 58161, the decision was to make the Stylist as WYSIWYG as possible. Works with limits of course, for example very large or small font sizes (handled well). I'd discuss each questionable attribute separately.
Comment 18 Heiko Tietze 2022-06-29 07:10:55 UTC
With a dark theme the control background is dark while the document color depends on the options. That results in black on white for automatic font color at the document and white on dark grey at the Stylist. Using explicitly black makes it unreadable.

Possible solution is to set the m_xTreeBox->set_background(svtools::ColorConfig().GetColorValue(svtools::DOCCOLOR).nColor); in StyleList::Initialize(). Works for gen/gtk3 but not kf5 as the treeview never fills the whole scrollwindow behind (haven't tested other systems).

Worse problem is the expander icon loaded via mpTreeImpl->SetExpandedEntryBmp( pEntry, maDefaultExpandedImage ); in TreeControlPeer::createEntry() resp. SvTreeListBox::SetExpandedEntryBmp(). The static image will not adjust to the entry color (I suspect trouble for alternating row colors too) and will become white on white.