Bug 532184 - [Dark Theme] Context help in wizards not styled correctly in dark theme
Summary: [Dark Theme] Context help in wizards not styled correctly in dark theme
Status: REOPENED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.8   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Platform-UI-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-08 09:57 EST by Matthias Becker CLA
Modified: 2021-12-23 10:41 EST (History)
6 users (show)

See Also:


Attachments
Screencast showing the issue (120.73 KB, image/png)
2018-03-08 09:57 EST, Matthias Becker CLA
no flags Details
Help in Help-View is styled correctly (358.67 KB, image/png)
2018-03-08 09:58 EST, Matthias Becker CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Becker CLA 2018-03-08 09:57:49 EST
Created attachment 273039 [details]
Screencast showing the issue

The context help in wizards (when pressing the ?-Button on the Wizard) is not styled correctly. See attached screenshot.
The help-view on the other is styled correctly already.
Comment 1 Matthias Becker CLA 2018-03-08 09:58:10 EST
Created attachment 273040 [details]
Help in Help-View is styled correctly
Comment 2 Matthias Becker CLA 2018-03-09 08:38:30 EST
I debugged this and found the root cause.

The issue is in FormToolkit#createScrolledForm. During the creation of the ScrolledForm the theme engine correctly sets the colors.
Then the FormToolkit overwrites them again by the call to  form.setBackground(colors.getBackground());

For debugging purposes I changed the WizardDialog to be modeless by changing in the constructors the line.
  setShellStyle(SWT.CLOSE | SWT.MAX | SWT.TITLE | SWT.BORDER | SWT.APPLICATION_MODAL | SWT.RESIZE | getDefaultOrientation());
to 
  setShellStyle(SWT.CLOSE | SWT.MAX | SWT.TITLE | SWT.BORDER | SWT.MODELESS | SWT.RESIZE | getDefaultOrientation());
After this you can use the CSS-Spy to inspect the tray.

When I open the CSS-Scratchpad while the wizard tray is open (and is showing the partly white background as in attachment 273039 [details]) and press "Apply" in the "CSS Scratch pad" without putting any additional CSS in there the styling of the Tray is corrected. This shows that the CSS is correct (and applied correctly in the first place) but colors are changed back by the FormToolkit.

This is the same issue as in https://bugs.eclipse.org/bugs/show_bug.cgi?id=514318#c10

One possible fix could be to somehow suspend the CSS-Engine (at least the property setting part) between the beginn of the FormToolkit#createScrolledForm method and
let the CSS-Engine apply it's property changes directly before the method returns.
By this the "wrong" colors set with 
 form.setBackground(colors.getBackground());
 form.setForeground(colors.getColor(IFormColors.TITLE)); 
would be overwritten with the correct values from the CSS.

But this is just a first thought. I don't know if this would have other side-effect and I also don't know if the CSS-Engine does provide the needed functions to delay the execution of the property-setting temporarily.


Adding Brian de Alwis and Till Brychcy as they may have knowledge in the CSS and Forms area. 
Brian and Till: Any remarks from your side?
Comment 3 Brian de Alwis CLA 2018-03-09 08:55:40 EST
I wonder if a saner approach might be to change how the Forms toolkit retrieves colours to consult the CSS engine?
Comment 4 Matthias Becker CLA 2018-03-09 09:01:20 EST
https://bugs.eclipse.org/bugs/show_bug.cgi?id=531422 had the same issue.
There is could be work-arounded by the fact that JDT set the foreground / background colors to null ( see JavaPlugin#getDialogsFormToolkit).

I tried the same pattern by setting the background color to null in HelpTray#createContents and adding a null-check in FormToolkit#createScrolledForm so that the background is only set when getBackground() does not return null.
This looked good in the dark theme (because the CSS engine does set all the colors). But this leads to artefact in the light theme (as there the CSS engine does not set all the colors.

So this really is a conceptual issue with the CSS styling and the FormToolkit.

Any ideas how to solve this?
Comment 5 Matthias Becker CLA 2018-03-09 09:01:47 EST
(In reply to Brian de Alwis from comment #3)
> I wonder if a saner approach might be to change how the Forms toolkit
> retrieves colours to consult the CSS engine?

How could that be done?
Comment 6 Brian de Alwis CLA 2018-03-12 10:36:31 EDT
My thoughts were that we could have FormColors#initialize*() somehow pull values from the CSS with something like org.eclipse.e4.ui.internal.workbench.swt.CSSRenderingUtils#getCSSValue().  Ideally we'd do this in such a way that neither org.eclipse.ui.forms nor org.eclipse.e4.ui.workbench.swt are exposed to each other.

Fundamentally the problem is that the we want to allow the CSS to overrule the SWT System/OS default colours.  Ideally this the mechanism should be useful for other widget libraries too.

Here's one approach that might work:

  - have the SWT CSS engine add a colour-lookup dictionary on Display that is populated from the CSS (via the getData()/setData())

  - FormColors#getSystemColor() will first try looking up a colour in that dictionary

It might be worth having that colour-lookup be provided with a context object, like the FormColors class name, so that it could use that object to key off the CSS (e.g., specific colours for Forms).

What do you think?
Comment 7 Matthias Becker CLA 2018-03-12 11:16:05 EDT
(In reply to Brian de Alwis from comment #6)
> My thoughts were that we could have FormColors#initialize*() somehow pull
> values from the CSS with something like
> org.eclipse.e4.ui.internal.workbench.swt.CSSRenderingUtils#getCSSValue(). 
> Ideally we'd do this in such a way that neither org.eclipse.ui.forms nor
> org.eclipse.e4.ui.workbench.swt are exposed to each other.
> 
> Fundamentally the problem is that the we want to allow the CSS to overrule
> the SWT System/OS default colours.  Ideally this the mechanism should be
> useful for other widget libraries too.
> 
> Here's one approach that might work:
> 
>   - have the SWT CSS engine add a colour-lookup dictionary on Display that
> is populated from the CSS (via the getData()/setData())
> 
>   - FormColors#getSystemColor() will first try looking up a colour in that
> dictionary
> 
> It might be worth having that colour-lookup be provided with a context
> object, like the FormColors class name, so that it could use that object to
> key off the CSS (e.g., specific colours for Forms).
> 
> What do you think?

As far as I understand the CSS engine does not have fixed values for e.g. a Label but the color is dependent also on the parents of the label (a label inside a composite my have a different color then a label inside a composite inside another composite).
But FormToolkit currently does some color-magic based on system colors.
Does that work together?

Keep in mind: FormToolkit is not only used on form editor but also on "normal" SWT dialogs.
Comment 8 Lars Vogel CLA 2018-03-12 11:17:22 EDT
A simpler way might be to use preferences in forms for this and use CASE to style them. 

Also, forms cannot have a dependency to the CSS engine as it is used standalone.
Comment 9 Matthias Becker CLA 2018-03-12 11:24:36 EDT
(In reply to Lars Vogel from comment #8)
> A simpler way might be to use preferences in forms for this and use CASE to
> style them. 
So you mean to replace all the magic in initializeColorTable() with definitions from the different CSSes?

> Also, forms cannot have a dependency to the CSS engine as it is used
> standalone.
What means "standalone"?
Comment 10 Lars Vogel CLA 2018-03-12 12:39:58 EDT
(In reply to Matthias Becker from comment #9)
> (In reply to Lars Vogel from comment #8)
> > A simpler way might be to use preferences in forms for this and use CASE to
> > style them. 
> So you mean to replace all the magic in initializeColorTable() with
> definitions from the different CSSes?
> 
> > Also, forms cannot have a dependency to the CSS engine as it is used
> > standalone.
> What means "standalone"?

Look in the manifest for its dependencies, I think it only requires SWT and maybe JFace.
Comment 11 Brian de Alwis CLA 2018-03-12 15:12:01 EDT
> As far as I understand the CSS engine does not have fixed values for e.g. a Label but the color is dependent also on the parents of the label (a label inside a composite my have a different color then a label inside a composite inside another composite).
> But FormToolkit currently does some color-magic based on system colors.
> Does that work together?

Sorry, I'm doing some wild hand-waving.  At its essence what I'm suggesting is adding a level of indirection between how FormToolkit/FormColor initializes its colour values from SWT to allow these values to be over-ridden with CSS.

My suggestion above was to add a colour look-aside table, and provide an implementation that can pull values from the CSS.  "Fixed" values can be provided in the CSS by querying against some fixed class. 

Lars' suggestion (IIUC) is to instead pull values from colour preferences, and thereby benefit from the existing preferences bridging support in the CSS.  Lars' suggestion seems easier.
Comment 12 Eclipse Genie CLA 2020-05-16 11:55:30 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug.

If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.
Comment 13 Lars Vogel CLA 2020-09-10 02:40:25 EDT
Still valid