apache / netbeans

Apache NetBeans
https://netbeans.apache.org/
Apache License 2.0
2.62k stars 840 forks source link

Alter FlatLaf tab colouring to address focus visibility. #4286

Closed neilcsmith-net closed 2 years ago

neilcsmith-net commented 2 years ago

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 https://github.com/apache/netbeans/pull/4286#issuecomment-1171229094

flatlaf-light flatlaf-dark

eirikbakke commented 2 years ago

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 commented 2 years ago

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! :wink:
  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 commented 2 years ago

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 https://github.com/apache/netbeans/pull/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 commented 2 years ago
  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 commented 2 years ago

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 commented 2 years ago
  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 commented 2 years ago

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

Chris2011 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 https://github.com/apache/netbeans/pull/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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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.

DevCharly commented 2 years ago

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 commented 2 years ago

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

neilcsmith-net commented 2 years ago

Thanks @DevCharly @mbien Agreed! I'll have another look now. I swapped to lighter because I was running out of value (HSV) space to contrast, but I agree keeping it darker would be better.

Was intending to look at consistency with multitabs separately.

neilcsmith-net commented 2 years ago

OK, tried reverting to a darker tab background on FlatLaf dark. 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. Or now too close to editor background?

flatlaf-dark4

Also changed hover colours to be (or derive from) @componentBackground so they don't merge. Dark laf goes lighter on hover too, which is more like the menu than the previous tab design.

Also checked with various alternative @accentBaseColor settings.

If this looks better, I'll squash again for merging. @eirikbakke @mbien @DevCharly ?

Chris2011 commented 2 years ago

shouldn't the sub tabs also look the same as the parent tabs?

image
neilcsmith-net commented 2 years ago

@Chris2011 IMO, no. The idea is to make the primary window tabs / regions stand out - making sub tabs similar would make it more cluttered and harder to see. However, I would like to see if we can make the tab highlight colour follow which region is focused - ie. should also be light grey here. But we already have that problem.

Chris2011 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

A better mockup summarizing my proposed improvements...

MoreMockup

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

neilcsmith-net commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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) :laughing: -

flatlight6 flatdark6

eirikbakke commented 2 years ago

If we're going to start drawing borders around tabs, then we'll need the improvements from https://github.com/apache/netbeans/pull/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 https://github.com/apache/netbeans/pull/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 https://github.com/ultorg/public_issues/issues/11 ). But that would be for a future PR.

neilcsmith-net commented 2 years ago

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

eirikbakke commented 2 years ago

@neilcsmith-net Wait, you don't think the lower part of https://user-images.githubusercontent.com/886243/176727415-beb3faed-755d-4e33-93bc-8b36d61cfe48.png looks better?

neilcsmith-net commented 2 years ago

@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 commented 2 years ago

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.

neilcsmith-net commented 2 years ago

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 commented 2 years ago

I did a build with this patch and https://github.com/apache/netbeans/pull/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!