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

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

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.

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

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

(In reply to Mike Kaganski from comment #5)

(and which of multiple page styles should be used here?)

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

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

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);
 }

(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!

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.

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.

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.

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

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

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.

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.

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.

*** Bug 138979 has been marked as a duplicate of this bug. ***

*** Bug 153504 has been marked as a duplicate of this bug. ***

With recent improvements in macOS and Windows OS dark mode support, this issue is not Linux-specific anymore.
Changing meta bugs accordingly. Also affects GNOME, so removing KDE from meta.

*** Bug 157175 has been marked as a duplicate of this bug. ***

(In reply to steve from comment #24) 
> Works find on macOS
Still an issue in recent trunk build. Did you change a style's font colour to a dark colour, instead of the default "Automatic"?

Repro on Manjaro KDE with LibreOffice 24.2.0.3