eclipse-platform / eclipse.platform.ui

Eclipse Platform
https://projects.eclipse.org/projects/eclipse.platform
Eclipse Public License 2.0
81 stars 188 forks source link

SWT Dark Theme ToolItem Toggle Buttons not showing down state #467

Open mikevwdriver opened 1 year ago

mikevwdriver commented 1 year ago

I am using the 2022-12 RC1 on Windows 10. Toggle buttons in tool bars and even regular toggle buttons are not showing their down state as expected.

image You can't tell, but the sync with editor button is currently pressed down.

The same issue can be seen in the perspective chooser: image

mikevwdriver commented 1 year ago

@vogella I've identified the commit that caused this issue: c07ab5389e1c9b75e46af9dd7cf0c522e35bba64

Before this commit, ToolItem toggle buttons had a color that indicated their down state, starting with this commit, the down state is no longer apparent on Dark theme.

edit: It does appear that regular SWT toggle buttons were broken by a different commit than the ToolItem toggle buttons. For this separate issue, I've opened https://github.com/eclipse-platform/eclipse.platform.swt/issues/483

mikevwdriver commented 1 year ago
// ToolItem prevents itself from repaints if the same color is set
((ToolItem) widget).setBackground(newColor);

If I remove this one line from CSSPropertyBackgroundSWTHandler.java, the ToolItems are styled correctly.

mikevwdriver commented 1 year ago

Okay, more information. This is a bug that affects the standard Eclipse IDE. Our product has it's own custom CSS, it was affected by this change as well, however, simply by removing all instances of ToolItem in our dark theme CSS files, we are no longer affected by this issue.

I took a look at the platform CSS and although it does not directly reference ToolItem, I suspect it is inheriting a rule which causes the background to be explicitly set, which will unfortunately have an effect on the toggled/checked state.

humphreygao commented 1 year ago

https://github.com/eclipse-platform/eclipse.platform.ui/issues/340

de-jcup commented 1 year ago

Is there any update on fixing this issue?

I am using now eclipse 2022-12 (nearly only JDT) and I am having really big problems with this:

At Console, JUnit, Project explorer and many more views it is not possible to determine if an icon inside a view toolbar is enabled or disabled.

Using the dark mode with Linux Mint (GTK) variants, there is absolutely no way to check this. Especially when it comes to the console view this is very irritating (e.g. the "Show console log when standard out changes"). There is no menu inside where I can check the states (as a workaround):

image

(remark: i toggled the states on/off and it always look the same...)

de-jcup commented 1 year ago

Hmm... tried out oomp-installer + platform IDE - starting Eclipse 2023-03 (master) I had following:

image

The toolbar icons are now correct rendered with Theme "dark". Enabled ToggleIcons have now a border again, so it's clear which state they have.

If this looks same in JDT (and I think so) than this has been already fixed for 2023-03

trancexpress commented 1 year ago

Some of the screenshots here show many views opened in the same part stack, i.e. there are many tabs. When there are many tabs, the toolbar buttons will be painted not on the tab bar, but below it. You might want to check this case as well, just in case (since the last screenshot I see has only a few tabs and the toolbar buttons are painted on the tab bar).

merks commented 1 year ago

It's true that some seem to paint their check state and others don't:

image

de-jcup commented 1 year ago

Yes It is still a problem with upcoming 2023-03. It was only correct rendered, because the toolbar was "on top". When reducing the size of the window, the toolbar is no longer rendered with the light grey background, but uses the dark one: The Border has the same color like the dark background - so border for selection becomes invisible. image

l-uuz commented 1 year ago

Still present in 2023-03. Is there a workaround? This is really annoying, e.g. with the button "Skip all breakpoints".

trancexpress commented 1 year ago

Using GTK+ Inspector and comparing toolbar buttons with Breeze Dark on KDE:

  1. The main toolbar buttons have background color (252,252,252,0.125). There is a theme .css line associated with the color.
  2. View toolbar buttons have background color (81,86,88). There is no theme .css line associated with the color.

In an SWT example for toolbar button toggles, (252,252,252,0.125) seems to be the expected background color. Makes sense since the for the main toolbar the toggle state is visible. For the view toolbars, only the border is visible for me.

public class ToolbarToggleExample {

    public static void main(String[] args) {
        Display display = new Display();
        Shell shell = new Shell(display);
        shell.setSize(400, 300);
        shell.setText("Toolbar with Images");
        shell.setLayout(new RowLayout(SWT.VERTICAL));
        ToolBar toolBar = new ToolBar(shell, SWT.HORIZONTAL);
        createToolItem(toolBar, SWT.CHECK, "Check One", "This is check one");
        createToolItem(toolBar, SWT.CHECK, "Check Two", "This is check two");
        shell.open();
        while (!shell.isDisposed()) {
            if (!display.readAndDispatch()) {
                display.sleep();
            }
        }
        display.dispose();
    }

    private static ToolItem createToolItem(ToolBar parent, int type, String text, String toolTipText) {
        ToolItem item = new ToolItem(parent, type);
        item.setText(text);
        item.setToolTipText(toolTipText);
        return item;
    }
}

The unexpected color Color {81, 86, 88, 255} is set here:

"main" #1 prio=6 os_prio=0 cpu=1458.75ms elapsed=34.43s tid=0x00007f58ac015460 nid=0x8afa at breakpoint [0x00007f58b0ffb000]
   java.lang.Thread.State: RUNNABLE
        at org.eclipse.swt.widgets.ToolItem.setBackground(ToolItem.java:1111)
        at org.eclipse.e4.ui.css.swt.properties.css2.CSSPropertyBackgroundSWTHandler.applyCSSPropertyBackgroundColor(CSSPropertyBackgroundSWTHandler.java:76)
        at org.eclipse.e4.ui.css.core.dom.properties.css2.AbstractCSSPropertyBackgroundHandler.applyCSSProperty(AbstractCSSPropertyBackgroundHandler.java:43)
        at org.eclipse.e4.ui.css.swt.properties.css2.CSSPropertyBackgroundSWTHandler.applyCSSProperty(CSSPropertyBackgroundSWTHandler.java:43)
        at org.eclipse.e4.ui.css.core.impl.engine.AbstractCSSEngine.applyCSSProperty(AbstractCSSEngine.java:746)
        at org.eclipse.e4.ui.css.core.impl.engine.AbstractCSSEngine.applyStyleDeclaration(AbstractCSSEngine.java:552)
        at org.eclipse.e4.ui.css.core.impl.engine.AbstractCSSEngine.applyStyles(AbstractCSSEngine.java:426)
        at org.eclipse.e4.ui.css.core.impl.engine.AbstractCSSEngine.applyStyles(AbstractCSSEngine.java:374)
        at org.eclipse.e4.ui.css.swt.engine.CSSSWTApplyStylesListener.lambda$0(CSSSWTApplyStylesListener.java:31)
        at org.eclipse.e4.ui.css.swt.engine.CSSSWTApplyStylesListener$$Lambda$317/0x00000008004ff660.handleEvent(Unknown Source)
        at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
        at org.eclipse.swt.widgets.Display.sendEvent(Display.java:5855)
        at org.eclipse.swt.widgets.Display.sendEvent(Display.java:5847)
        at org.eclipse.swt.widgets.Display.runSkin(Display.java:5146)
        at org.eclipse.swt.widgets.Composite.computeSizeInPixels(Composite.java:262)
        at org.eclipse.swt.widgets.Control.computeSize(Control.java:838)
        at org.eclipse.swt.widgets.Control.pack(Control.java:1610)
        at org.eclipse.swt.widgets.Control.pack(Control.java:1585)
        at org.eclipse.e4.ui.workbench.renderers.swt.StackRenderer.addTopRight(StackRenderer.java:786)
        at org.eclipse.e4.ui.workbench.renderers.swt.StackRenderer.createWidget(StackRenderer.java:673)
        ...

The used color is defined here:

bundles/org.eclipse.ui.themes/css/dark/e4-dark_ide_colorextensions.css:ColorDefinition#org-eclipse-ui-workbench-DARK_BACKGROUND {
bundles/org.eclipse.ui.themes/css/dark/e4-dark_ide_colorextensions.css- color: #515658;

In org.eclipse.ui.themes/css/dark/e4-dark_partstyle.css, the color is used for buttons (toolbar buttons have nested buttons in GTK3):

Shell,
Composite, ScrolledComposite, ExpandableComposite, Canvas, TabFolder, CLabel, Label,
CoolBar, Sash, Group, RefactoringLocationControl, ChangeParametersControl, Link, FilteredTree,
ProxyEntriesComposite, NonProxyHostsComposite, DelayedFilterCheckboxTree,
Splitter, ScrolledPageContent, ViewForm, LaunchConfigurationFilteredTree,
ContainerSelectionGroup, BrowseCatalogItem, EncodingSettings,
ProgressMonitorPart, DocCommentOwnerComposite, NewServerComposite,
NewManualServerComposite, ServerTypeComposite, FigureCanvas,
DependenciesComposite, ListEditorComposite, WrappedPageBook,
CompareStructureViewerSwitchingPane, CompareContentViewerSwitchingPane,
QualifiedNameComponent, RefactoringStatusViewer,
MessageLine,
Button /* SWT-BUG: checkbox inner label font color is not accessible */,
Composite > *,
Composite > * > *,
Group > StyledText {
    background-color:'#org-eclipse-ui-workbench-DARK_BACKGROUND'; 
    color:'#org-eclipse-ui-workbench-DARK_FOREGROUND'; 
}

The code in ToolItem.updateStyle() that sets the background color:

    if (background != null) {
        css +=
            "button { background-image: none; background-color: " + display.gtk_rgba_to_css_string (background.handle) + "; }";
    }

Seems odd that this would overwrite the other states (disabled, checked, etc.) from the theme... Maybe all of the .css for toolbar buttons is overriden with this.

The change was added for: https://bugs.eclipse.org/bugs/show_bug.cgi?id=579470

I don't understand why this styling doesn't apply to the main toolbar. Maybe because the .css specifies CoolBar but not ToolBar? I tried adding ToolBar though and the main toolbar still has no problem.

trancexpress commented 1 year ago

@nnemkin can you take this over? I'm not sure what the best approach here is. For Breeze Dark, this behavior emulates what the main toolbar does:

diff --git a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/ToolItem.java b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/ToolItem.java
index cd970973ab..5673e8676e 100644
--- a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/ToolItem.java
+++ b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/ToolItem.java
@@ -1602,7 +1602,9 @@ void updateStyle () {
                css += parent.cssForeground;
        }
        if (background != null) {
-               css += "button { background-image: none; background-color: " + display.gtk_rgba_to_css_string (background.handle) + "; }";
+               css +=
+                       "button { border: none; background-image: none; background-color: " + display.gtk_rgba_to_css_string (background.handle) + "; }\n" +
+                       "button:checked { background-color: transparent; }\n";
        }

        if (GTK.GTK4) {

But if the toolbar is drawn on the same line as tabs in a part stack, the color difference is not enough to see the toggled-off button. Since the part stack line has the same color as the toolbar button background.

It doesn't look like the platform UI .css can provide a toggled-off background color. Hard-coding one doesn't seem like a good approach either. E.g. this still doesn't yield good results and is not consistent with the main toolbar:

diff --git a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/ToolItem.java b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/ToolItem.java
index cd970973ab..819b9c4a26 100644
--- a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/ToolItem.java
+++ b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/ToolItem.java
@@ -1602,7 +1602,12 @@ void updateStyle () {
                css += parent.cssForeground;
        }
        if (background != null) {
-               css += "button { background-image: none; background-color: " + display.gtk_rgba_to_css_string (background.handle) + "; }";
+               Color disabledBackgroundColor = new Color(background.getRGB(), 127);
+               String disabledBackgroundColorCss = display.gtk_rgba_to_css_string (disabledBackgroundColor.handle);
+               disabledBackgroundColor.dispose();
+               css +=
+                       "button { background-image: none; background-color: " + display.gtk_rgba_to_css_string (background.handle) + "; }\n" +
+                       "button:checked { background-color: " + disabledBackgroundColorCss + "; }\n";
        }

        if (GTK.GTK4) {

IMO the first step here is to make the main toolbar behave the same as the view toolbar. As well as, to ensure the view toolbar buttons are visible both when drawn in the view client area and when drawn on the tab line of the view part stack.

Then a color can be chosen with which using background-color: transparent; for toggled-off buttons ensures visibility both for toggled-on and toggled-off state.

de-jcup commented 1 year ago

It has been some time when I tried to fix the problem and maybe it got lost, so I bring up my thoughts here:

In my draft PR https://github.com/eclipse-platform/eclipse.platform.ui/pull/538 I fixed this problem, but I was forced to remove https://github.com/eclipse-platform/eclipse.platform.ui/commit/c07ab5389e1c9b75e46af9dd7cf0c522e35bba64 to get it working. (done in second commit of my draft PR, you can see the new (and good looking )UI behavior inside a picture at https://github.com/eclipse-platform/eclipse.platform.ui/pull/538#issuecomment-1416848767).

I am not very good in SWT or in SWT CSS styling - I was/am stuck at this point. And I am running out of time. But maybe my draft PR is an idea for somebody else?

@akurtakov , @vogella : Maybe the remove or an improvement of https://github.com/eclipse-platform/eclipse.platform.ui/commit/c07ab5389e1c9b75e46af9dd7cf0c522e35bba64 could be an option? The current situation is extreme annoying (at least for myself) .

akurtakov commented 1 year ago

I'm sorry but I can not dedicate time to look into this one.