Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skins :: adjust styles of skin context menus, tooltip, ... #2337

Merged
merged 12 commits into from
Nov 15, 2019

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Oct 29, 2019

Synchronise styles of skin right-click menus, tooltip, ...

This should definitely be included in the 2.3 release because it makes the entire regular GUI interaction a consistent experience. No more OS themes crushing in, except popup windows like file dialogs and warnings.

It affects

  • Tracks table menu
  • hotcue menu
  • cover art menu
  • tooltips
  • effect selector drop-down
  • table header menu
  • editable fields (Search box, beat spinboxes, library items)
  • skin settings menu (Tango, LateNight, maybe Deere)
  • sidebar menu (with allow skins to style sidebar context menus #2338 merged)

Also I fixed the appearance of effect selector drop-down in Shade.

qmenu-styles_LateNight

ToDo

  • use relative font size where applicable? Nope, maybe later on..
  • test with various themes, also on Windows and MacOS
  • style indeterminate (partially checked) checkboxes (tracks' Crates menu)
  • style editable fields
  • Shade: fix library selection colors in all color schemes
  • include AutoDJ trasition mode drop-down from make better use of intro/outro markers with AutoDJ #2103
  • remove WEffectSelector style hack from MacOS stylesheets
  • fix (closed) WEffectSelector background on MacOS

@ronso0
Copy link
Member Author

ronso0 commented Nov 1, 2019

related Zulip discussion:
Skin accessibility / font size

@ronso0 ronso0 changed the title Skins :: adjust styles of skin context menus, tooltip, ... [WIP] Skins :: adjust styles of skin context menus, tooltip, ... Nov 2, 2019
@ronso0
Copy link
Member Author

ronso0 commented Nov 2, 2019

marked WIP until I get some feedback on the fonts in Zulip Skin accessibility / font size

The rest is ready, please test. Tracks context menu, cover menu, hotcue menu, table headers ...

@ronso0
Copy link
Member Author

ronso0 commented Nov 4, 2019

marked WIP until I get some feedback on the fonts in Zulip Skin accessibility / font size

If there's none, let me know when it's okay to merge.

@ronso0
Copy link
Member Author

ronso0 commented Nov 5, 2019

anyone tested this yet?

@ronso0 ronso0 mentioned this pull request Nov 8, 2019
2 tasks
@Be-ing
Copy link
Contributor

Be-ing commented Nov 11, 2019

Merge conflicts have developed.

@Be-ing
Copy link
Contributor

Be-ing commented Nov 11, 2019

use relative font size where applicable?

I don't understand what you mean. Relative to what? What is the problem and why should it delay merging this?

@ronso0
Copy link
Member Author

ronso0 commented Nov 11, 2019

use relative font size where applicable?

I don't understand what you mean. Relative to what? What is the problem and why should it delay merging this?

Relative to default system font size: 1em = 100% system font size
It doesn't necessarily block this, but I can't really estimate the side effects of the changes so some feedback / advice would be nice.

Besides, I didn't receive any feedback on this PR yet. Testing on all OS would be appreciated, as well as testing with scale factors !=100%. Please provide a few screenshots when you do so, so I can compare if it looks somewhat similiar to the intended design at 100%.
Thanks!

@Be-ing
Copy link
Contributor

Be-ing commented Nov 11, 2019

We use px units all over skins and this causes no problems because Qt scales them all together with the scale factor.

@ronso0 ronso0 changed the title [WIP] Skins :: adjust styles of skin context menus, tooltip, ... Skins :: adjust styles of skin context menus, tooltip, ... Nov 11, 2019
@ronso0
Copy link
Member Author

ronso0 commented Nov 12, 2019

We use px units all over skins and this causes no problems because Qt scales them all together with the scale factor.

Sure. I stumbled over em at one place in Deere's style.qss and I thought about relative font sizes for the first time IIRC.
An example where relative font sizes make sense:
Mixxx user with 14" FullHD screen. Pixel density is quite high and you can imagine he/she scaled up the system font to let's say 125% or even 150%. Otoh you wouldn't want to scale up Mixxx this way as most icons look blurry then, and actually the font size in the decks is okay to read, while in the library it can be scaled individually. Point is, we could use relative font sizes for the skin settings menu and the table header, because we also could assume the FullHD screen has enough space without menu items being squeezed.
Right now we use absolute font sizes to make everything fit within the minimal window size we determind for each of our skins. Use Tango or Deere (unscaled) on a FHD screen to see what I mean.

@ronso0
Copy link
Member Author

ronso0 commented Nov 12, 2019

I'll resolve the conflicts soonish, most of them are about #fadeModeCombobox I guess.
Please test. No need to compile, skin changes only.

@ronso0
Copy link
Member Author

ronso0 commented Nov 12, 2019

screenshot including styled skin menu:
qmenu-styles_LateNight

@Be-ing
Copy link
Contributor

Be-ing commented Nov 12, 2019

In all skins except Deere, I think the headers for track table columns are somewhat hard to read. Could you make the text colors a bit brighter?

In Tango, the "intermediate" checked box in the Crate menu is difficult to distinguish from unchecked. In the other skins it is okay.

The right click menu in the library search bar has some icons that are blurry when scaled. Also, the text of "Select All" and "Ctrl + A" overlaps. These are very minor issues and might be out of the scope of this PR. I don't think I've ever right clicked in the search box before testing this PR.
image

I find the text for hovered menu items in Deere a little hard to read.

@ronso0
Copy link
Member Author

ronso0 commented Nov 12, 2019

The right click menu in the library search bar has some icons that are blurry when scaled. Also, the text of "Select All" and "Ctrl + A" overlaps. These are very minor issues and might be out of the scope of this PR. I don't think I've ever right clicked in the search box before testing this PR.

Please try the right-click menu for selected/editable fields in the tracks table with master. The blurry icons are there as well, and I didn't touch any menu items except the checkboxes. I noticed the blurry OS icons when closing Mixxx at 200%.

Regarding the overlap, I'll check which other default menu types there are and how the items can be addressed.

@ronso0
Copy link
Member Author

ronso0 commented Nov 12, 2019

btw table headers are affected by the 'relative fonts' issue I tried to point out above: they don't scale with the library font size. But I think this can be fixed in c++

@ronso0
Copy link
Member Author

ronso0 commented Nov 12, 2019

In all skins except Deere, I think the headers for track table columns are somewhat hard to read. Could you make the text colors a bit brighter?

My impression is quite the opposite: the header font is regular in Deere, and bold in the other skins where it has a nice contrast that matches each skin (except in Shade Dark theme which I'll try to fix).
I'll compare this with master to check if there's a regression with this PR.

@Be-ing
Copy link
Contributor

Be-ing commented Nov 12, 2019

My impression is quite the opposite: the header font is regular in Deere, and bold in the other skins where it has a nice contrast that matches each skin (except in Shade Dark theme which I'll try to fix).

I think the font sizes are fine. It is the font colors that are somewhat hard to read.

@ronso0
Copy link
Member Author

ronso0 commented Nov 12, 2019

My impression is quite the opposite: the header font is regular in Deere, and bold in the other skins where it has a nice contrast that matches each skin (except in Shade Dark theme which I'll try to fix).

I think the font sizes are fine. It is the font colors that are somewhat hard to read.

It's strange it doesn't look okay for you, because I didn't change the font colors in this branch. I adapted them in Shade only to match the overall contrast of the color themes.
Can you post a screenshot just to make sure everything looks as it should on your system?

@ronso0
Copy link
Member Author

ronso0 commented Nov 13, 2019

While fixing the merge conflicts I also cleaned up a bit and managed to address the icons in editline menu and cue color menu.

@ronso0
Copy link
Member Author

ronso0 commented Nov 13, 2019

We had a separate stylesheet for MacOS which hacks around a bug with effect selector.
I just commented that out in Deere & LateNight. Could anyone on Mac please test if the bug still exists in Qt5?

@esbrandt
Copy link
Contributor

Tested on macos 10.14.6 (18G103)

  • This PR:
    Capto_Capture 2019-11-14_03-08-05_PM
    Capto_Capture 2019-11-14_03-06-31_PM

alt-text-1 alt-text-2

  • Master:
    alt-text-1 alt-text-2

  • Dont know if related to this PR. When editing hotcues by right-clicking on them in the waveform overview, the CUE label window inherits the wrong color with some skins.
    alt-text-3

QMenu::item:selected,
QMenu QCheckBox:selected,
QMenu QCheckBox:focus, /* selected by keyboard */
QMenu QCheckBox:hover /* mouse hover */ {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What i do think is, that at least on macOS, the styled menus are harder to read than before. Specially the active items. Try to look from some distance. With this fore-/ background colors, text has a color contrast ratio of 2.8:1, we should aim for at least 4.5 with such small fonts according to WCAG

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. On one hand, the pupose of this PR is to avoid those super bright context menus in our nighttime skins. Otoh I see the contrast can be increased for Deere.
How does the contrast feel like in the other skins? I think in LateNight at least it's sufficient consindering the overall contrast in the skin:
image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, thanks for testting!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the contrast for hovered menu items is okay in the other skins. IMO it is only Deere where it is a problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take care of this in a new PR since this one was merged prematurely.

@ronso0
Copy link
Member Author

ronso0 commented Nov 14, 2019

Tested on macos 10.14.6 (18G103)

I'm glad the checkmarks work in MacOS, but I'm puzzled why the (closed) effect selector is white in Deere...

@Be-ing
Copy link
Contributor

Be-ing commented Nov 14, 2019

It's strange it doesn't look okay for you, because I didn't change the font colors in this branch. I adapted them in Shade only to match the overall contrast of the color themes.

Okay, I thought you did adjust them. There is no pressing need to change them in this PR then. It's not bad as is, but I think there is room for improvement.

@ronso0
Copy link
Member Author

ronso0 commented Nov 14, 2019

On that topic: after @esbrandt mentioned WCAG I found this contrast tool:
WebAIM Contrast Checker
Did some tests with menu colors and we're all good, read: in most cases we're way above the suggessted minimal contrast ratio of 4,5:1

@daschuer
Copy link
Member

I can confirm it looks good here either. Thank you for taking care.

@daschuer
Copy link
Member

The contrast checker is a nice tool. Should we link it somewhere in our wiki?

@daschuer daschuer merged commit 418d992 into mixxxdj:master Nov 15, 2019
@Be-ing
Copy link
Contributor

Be-ing commented Nov 15, 2019

This was merged prematurely. @esbrandt and I both had a concern that had not been addressed; now a regression has been merged.

@ronso0
Copy link
Member Author

ronso0 commented Nov 15, 2019

yes, there are still items on the checklist.
And it lacks testing on the lowest version of MacOS we aim to support with 2.3 (1) because I suspect there we might still encounter te WEfectSelector bug that was the reason for the workaroud I removed from Deere & LateNight for testing purposes.
(1) which version is this btw? Anyone has this at the top of his head before I start digging?

@esbrandt
Copy link
Contributor

(1) which version is this btw? Anyone has this at the top of his head before I start digging?

10.11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants