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

Alter FlatLaf tab colouring to address focus visibility. #4286

Merged
merged 1 commit into from Jul 5, 2022

Conversation

neilcsmith-net
Copy link
Member

@neilcsmith-net neilcsmith-net commented Jun 27, 2022

Experiment Inspired by discussion in #3115 to see if concerns can be addressed (mostly) by colour changes rather than changing away from the existing flat design.

Improve colour contrast between tabs and background.
Move accent colour (underline) to top of tabs.
Make view tabs active / selected behaviour same as editor tabs.

NB. Latest revised images at #4286 (comment)

flatlaf-light
flatlaf-dark

@neilcsmith-net neilcsmith-net added the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Jun 27, 2022
@eirikbakke
Copy link
Contributor

eirikbakke commented Jun 27, 2022

This is good! A few proposed adjustments:

  1. Could you make the active editor tab the same color as the editor toolbar? (Either by adjusting the tab color or the toolbar color or both.)
  2. Can we have the tab background be the same in the sidebar and the editor area? I.e. get rid of the special background color for the "active" area. We already know which tab area is active by seeing which tab is active (and it's usually always the editor area anyway).
  3. Consider getting rid of the thick gray line on top of active-but-unfocused tabs. That will simplify the look, and make the focused tab stand out more.

@neilcsmith-net
Copy link
Member Author

Thanks for taking a look @eirikbakke I had already tried 1&3, but have revised.

  1. The problem is that either the editor tabs don't match the toolbar or the view tabs don't match their contents (or we have editor tab and view tabs different colours). Have matched with toolbar and moved the colours around a bit for more contrast. Couldn't make dark tab background dark enough so made it lighter instead, which I think looks a bit better anyway?
  2. Sorry, I think this is really important for showing focus, and I deliberately gave it a little more contrast. Fed up of typing in the wrong tab! 😉
  3. I tried without entirely already and didn't like it. Went to try again and accidentally used the background colour - think this works quite well.

flatlaf-light2
flatlaf-dark2

@eirikbakke
Copy link
Contributor

Thanks for the latest tweaks!

  1. The solution is to make the editor tab and the toolbar tab different colors; this is what I did in [NETBEANS-5928] Proposed improvements to FlatLAF tab components+borders/margins #3115 . Having the tab color match the background below is one of the most important visual cues for an active tab.
  2. I suspect that fixing (3) will solve the problem you describe here. Are you describing a split-window situation where you have two different rows of editor tabs?
  3. The problem with having a grey bar is that the unfocused tab looks too much like a focused tab, leading to the problem you described in (2). In peripheral vision, there is no color vision, and so grey and blue looks identical. I think only the focused tab should have a bar on top. You could also make it a brighter and more saturated blue.

@neilcsmith-net
Copy link
Member Author

  1. That presumes that all view tabs have that different background, which they don't. That's an issue to consider elsewhere IMO. Reduce the inconsistency further, not add to it!
  2. I'm talking about editor and view (eg. terminal) tabs. I don't find the bar by itself (no matter how bright we make it) enough of a focus hint. I want to keep the background colour follows focus behaviour.
  3. Yes, I agree, which is why it's effectively removed. Making it the background colour makes the tab slightly shallower than the container, with the bar effectively appearing on top. I don't see the second set of images makes the unfocused tab look focused in the same way the first set did. At the same time, I find current FlatLaf very difficult to see what the active tab is in an unfocused set.

@eirikbakke
Copy link
Contributor

eirikbakke commented Jun 27, 2022

  1. OK, one way or another, there will be some inconsistency here; some TopComponents will have toolbars and others will not.
  2. OK, I see the utility. It just looks bad with all of these different background colors.
  3. If you think the line is necessary, then consider: Should we not just have borders around the active tab, like in normal tab components? I'm not convinced that removing all borders is actually a good thing for the UI. Completely "flat" may be too minimal.

Adding a (4) here... it's a bit odd to have no visible boundary between the split pane drag area and the tab. Making this area the same darker grey as the tab background, as suggested in the other thread, would solve this problem. (Not sure I understood your objection there relating to the empty editor area; the empty editor background is not usually visible at all.)
boundary

@neilcsmith-net
Copy link
Member Author

  1. Yes, you can see this in your screenshot with the Output region. I don't think adding another level of inconsistency with multiple tab colours is the right approach. In some ways, I'd prefer the tab colour to be as my first image - it fixes the other problem you raise!
  2. Not many other options while not losing the current focus utility that FlatLaf has.
  3. There isn't a border, there's padding. If we wanted one, we could make it the same as the tab separator colour. I'm not against borders - I have an issue with adding gradients to a flat look and feel. If you look at the examples you shared from other software, a border alone (unless it's the accent colour) is not enough. Border or not, they work because of contrasting inactive tab background. @Chris2011 made that point too.
  4. I think it should be clear where the window regions are. So having the dividers the same colour as the toolbar and status bar makes sense unless we add yet another colour. FlatLaf already has this blurring problem. But if you want the active tab to be the toolbar colour (rather than current default), it's going to blend into those. Maybe we should tweak to draw the tab separator line at the start as well?

@neilcsmith-net
Copy link
Member Author

neilcsmith-net commented Jun 28, 2022

Moved inactive underline colour back to what it was in first image (and close to original colour). Kept tab background same as toolbar. Think it makes more sense of selected vs focused distinction to me. Final set of images -

flatlaf-light3

flatlaf-dark3

Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

I am happy that the line is on top so +1 from me.

pretty interesting that this all is done just by editing properties. Like CSS in a HashMap.

@neilcsmith-net neilcsmith-net added Need Squashing and removed do not merge Don't merge this PR, it is not ready or just demonstration purposes. labels Jun 28, 2022
@Chris2011
Copy link
Contributor

I like it, just to be a little croaker, this looks a bit broken. I would prefer to not see the right line due to the editor tabs also not shows it.

image

@neilcsmith-net
Copy link
Member Author

@Chris2011 thanks. Should be fixed, and work like before if a different selected background colour not set. Please try.

pretty interesting that this all is done just by editing properties. Like CSS in a HashMap.

@mbien yes, there's a wealth of stuff that can be styled by properties in a CSS fashion, as well as useful style functions - see https://www.formdev.com/flatlaf/properties-files/ Next steps might be - open some of that up so users can easily configure / create themes, and possibly look at the additional properties that are available for tabs upstream that our displayers don't support.

@eirikbakke
Copy link
Contributor

I think this is strictly an improvement upon the status quo, so I'll say go ahead and merge once the index check is added if needed.

I'll probably go back to #3115 and split out some smaller, uncontroversial PRs, to add the various configuration properties that were necessary to support bordered tabs (even if we don't use them in the default theme).

@neilcsmith-net
Copy link
Member Author

Thanks @eirikbakke Options for border and sizing would be really good. There's also a bunch of other configuration options in upstream FlatTabbedPaneUI that might be worth considering?

@eirikbakke
Copy link
Contributor

@neilcsmith-net Initially I just want to make sure that all the different variations can be supported by setting keys in FlatLAF.properties, e.g. by adding the properties needed to support borders. Making it easier to override FlatLAF.properties (without having to compile anything) might be good, though.

@neilcsmith-net
Copy link
Member Author

neilcsmith-net commented Jun 28, 2022

OK, squashed and updated. Will leave CI to do its thing. I'll merge in a couple of days if no-one gets there first or wants to add to conversation.

@mbien
Copy link
Member

mbien commented Jun 28, 2022

as general note:
if a PR is updated and travis didn't run yet, please cancel the old runs here:
https://app.travis-ci.com/github/apache/netbeans/pull_requests

(gh actions are configured to do this automatically, I don't know how to do that for travis I lied, I just found the setting in the repository config - It should cancel automatically from now on)

i just did this for this PR since travis is in slow mode again and a lot was in queue.

Copy link
Member

@DevCharly DevCharly left a comment

Choose a reason for hiding this comment

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

I like the light variant, but I'm not sure about the dark variant...

In the light theme, the color of the tab areas (red arrows in below screenshot) is clearly different to the background colors of surrounding components (green arrows), which makes the tab areas stand out. The selected tabs (blue arrows) are easy to recognize.

image

But in the dark theme, the inactive tab areas (red arrows in below screenshot) use background color that is very similar to surrounding components (green arrows), which makes the tab areas go under (e.g. on left side of below screenshot). The selected tabs (blue arrows) are not that easy to recognize as in the light theme IMHO. The active tabs area looks good.

image

I actually liked the darker "tab areas" on the screenshot in the first post better. Why did you switch to lighter tab areas?

@DevCharly
Copy link
Member

BTW please also test the Multi-Tabs implementation (see PR #1865).
The keys in the properties files start with nb.multitabs..
Multi-Tabs do not yet support nb.multitabs.underlineAtTop=true, but this should be easy to add to org.netbeans.core.multitabs.impl.TabDataRenderer

@mbien
Copy link
Member

mbien commented Jun 29, 2022

i was using this with the dark theme a few hours, here some observations:

  • the contrast of inactive tabs (text vs background) is a bit low which makes it more difficult to read compared to NB 14, intuitively this looks as if it would be a disabled component which can't be interacted with
  • inactive tab mousevoer renders with the same color as the components around it which looks as if the tab is gone just the text remains, i prefer the version of NB 14 there too in a side by side comparison

@Chris2011
Copy link
Contributor

Yeah, tabs in tabs are also a pattern that I don't like and hard to achieve from a design perspective. Just wanted to ask, thx for the clarification :)

@eirikbakke
Copy link
Contributor

eirikbakke commented Jun 29, 2022

Getting adequate contrast with Eirik's request to use @background for selected tab means we have to go quite dark (original image used @componentBackground). Still, I think it is probably better this way around.

Yes, I think this is good!

Or now too close to editor background?

That's fine; the editor background is not adjacent to the tab background.

As commented in-code, you can also drop the separator line after the last tab:

seplast

Moved inactive underline colour back to what it was in first image

Wait, why did this grey line on top of the active-but-unfocused tab pop back in? See my earlier comment about blue and grey being indistinguishable in peripheral vision.

@mbien
Copy link
Member

mbien commented Jun 29, 2022

is it intended that the tab content has no border? Empty tabs next to each other look like they are the same component (see usages + project tab). (If you dock them next to each other its even more visible)
no_border

@eirikbakke
Copy link
Contributor

eirikbakke commented Jun 29, 2022

@mbien I agree that this is a problem. Again, @neilcsmith-net it would be solved by making the split pane handles the same background color as the tab background. That's how it's done in both the Windows and the Aqua LAFs:

backgroundsame

Neil's objection was: "So having the dividers the same colour as the toolbar and status bar makes sense unless we add yet another colour." (EDIT:) I think this was a misunderstanding; I was not proposing to make it the same color as the toolbar, but the same color as inactive unfocused tabs, like this (Photoshop mockup):

sliderback

@eirikbakke
Copy link
Contributor

eirikbakke commented Jun 29, 2022

A better mockup summarizing my proposed improvements...

MoreMockup

But if you prefer I can open my own PR later...

@neilcsmith-net
Copy link
Member Author

@eirikbakke yes, think you're right on the split pane handles. Sorry, wrong call! Aqua has different colour for view tabs background and split pane handles here though, which is what I was on about.

This would also be helped by integrating your border fix, which would allow option of bringing borders back.

Also agree on right tab separator.

Still disagree on removing unfocused line, particularly with FlatLaf update allowing inner tabs to sync on focus.

Also not sure about changing the line colour unless you're proposing changing the accent colour?

@eirikbakke
Copy link
Contributor

eirikbakke commented Jun 30, 2022

Aqua has different colour for view tabs background and split pane handles here though, which is what I was on about.

Oh yeah, now I understand. That is indeed unnecessary!

This would also be helped by integrating your border fix, which would allow option of bringing borders back.

OK, I will experiment in a later PR after this one is merged.

Still disagree on removing unfocused line, particularly with FlatLaf update allowing inner tabs to sync on focus.

OK, as long as it's an intentional choice.

Also not sure about changing the line colour unless you're proposing changing the accent colour?

I guess, yes. It was just an opportunity to make the focus indication a little brighter. Not too important though.

@DevCharly
Copy link
Member

@eirikbakke not sure whether those thick borders look good. Especially the vertical ones.
Would be nicer to have thin borders/dividers.

Maybe we should consider extending MultiSplitPane to support thin dividers,
where only 1px is visible, but the user still has a 6px wide invisible drag area:

image

Some years ago I wrote an article on how to do this with JSplitPane:
https://www.formdev.com/blog/swing-tip-jsplitpane-with-zero-size-divider/

Then it could look like this (red arrows mark thin dividers):

image

@neilcsmith-net
Copy link
Member Author

Having now tried it, I agree with @DevCharly - the thick border appears too much. Pending a change like the above, I've reverted the border colours back to how they originally were, and added a fix to remove the extra top border from editor tab tabs. I've also added side borders when "underline" is at top. If border colours are set to @background will still show as earlier images.

Added code to remove last separator (which for editor tabs is from @eirikbakke ), and lowered the contrast of the unfocused tab underline a little, too.

Possibly final images (again) 😆 -

flatlight6
flatdark6

@eirikbakke
Copy link
Contributor

eirikbakke commented Jun 30, 2022

If we're going to start drawing borders around tabs, then we'll need the improvements from #3115 , to introduce configuration properties from these and to make sure the borders line up on HiDPI screens and so on. Perhaps this PR could be limited to things that can be improved with changes in FlatLAF.properties only, or changes compatible with #3115 ? This will be a big help when I later try to integrate the work from the latter.

The latest screenshots once again has the problem of the split pane area not having the same background as the inactive tab background. Mocked-up fix:

fixgap

Maybe we should consider extending MultiSplitPane to support thin dividers, where only 1px is visible, but the user still has a 6px wide invisible drag area:

Yes, this could be a good improvement! If we did this, we could also get rid of the 4px border at the edge of the window (which would fix e.g. the Fitt's law issue described at ultorg/public_issues#11 ). But that would be for a future PR.

@neilcsmith-net
Copy link
Member Author

As I said, split pane colour not changed but border put back based on @DevCharly comment which I agree with having tried it.

@eirikbakke
Copy link
Contributor

@neilcsmith-net
Copy link
Member Author

@eirikbakke honestly, no. Having tried it in its full context, I don't like the bleed of the tab background colour. And with the content borders going back to how they were it's not needed to differentiate regions either.

@eirikbakke
Copy link
Contributor

Alright, you can leave it at whichever overall solution you find most harmonious. If we end up making thin draggable sliders later, it will require some of these things to be redesigned in any case.

…sues.

Improve colour contrast between tabs and background.
Move accent colour (underline) to top of tabs and extend border up tab sides.
Make view tabs active / selected behaviour same as editor tabs.
Don't paint view tab separators around selected tab when different selected background colour set.
@neilcsmith-net neilcsmith-net changed the title Alter FlatLaf tab colouring to address visibility. Alter FlatLaf tab colouring to address focus visibility. Jul 1, 2022
@neilcsmith-net
Copy link
Member Author

Squashed everything again. Ready to merge when green from my perspective.

@eirikbakke thanks. The main reason it came up here at all was the need originally to hide the content borders to make overline and contiguous tab content work. Given other issues that raised and lack of consensus on how to fix, reverting to existing borders and limiting this PR solely to the tab regions (inc. minimal fixes to render correctly) made more sense. I also changed my mind working with it - things sometimes seem right in a static screenshot, but not in use.

@eirikbakke
Copy link
Contributor

I did a build with this patch and #4298. Before/after screenshots on my Windows 11 laptop at 150% DPI scaling:

Before Neil's PR, light
After Neil's PR, light

Before Neil's PR, dark
After Neil's PR, dark

I'm fine with merging this!

@mbien mbien added Look and Feel UI User Interface labels Jul 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Look and Feel UI User Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants