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

Editor: restore highlightModifiedTabs functionality #12367

Conversation

FernandoAscencio
Copy link
Contributor

What it does

Closes #12185

Use of box-shadow parameter causes several issues when several box-shadow parameters apply to a single element.
Specially since one parameter is used to manage two independent visual cues (borderTop and borderBottom)
There are situations when the top border will have no assigned color and this will cause the entire box-shadow parameter to not render. Meaning that a missing color for borderTop causes an active borderBottom to disappear and vice-versa. To add to the issue, the parameter will overwrite other applicable box-shadow, so you can end up with other stylings not showing when they should.

There are two possible solutions:

  • First one involves a complicated system that checks if the colors for top and bottom exist and then builds a box-shadow parameter out of the results and finally adding it to the style sheet.
  • The second is changing box-shadow for border-top and border-bottom decoupling their dependency on one another.
    For editor tabs, there is no real difference between using box-shadow and border. Thus, this commit changes the use of box-shadow to border.

This commit also fixes a tangential issue with High Contrast themes and how borders did not properly render when workbench.editor.highlightModifiedTabs was active.

How to test

Similarly to #12185

  1. Open Theia (browser or electron), set auto-save to off and workbench.editor.highlightModifiedTabs to true
  2. Select a file, modify it and see the top border appear.

Review checklist

Reminder for reviewers

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

For editor tabs, there is no real difference between using box-shadow and border.

Unfortunately there is: While box-shadow has absolutely no influence on the layout of DOM elements, border does influence the layouting of child elements. This is visible in this PR when quickly switching between modified and unmodified document state. I've visualized the location of the tab label with a red line in the following:

image

image

Note the way that the top border moves the content of the tab bar down by 1 pixel. I think a better box shadow solution is necessary (or more css to offset the border).

@msujew msujew added the ui/ux issues related to user interface / user experience label Apr 3, 2023
@FernandoAscencio FernandoAscencio force-pushed the fa/restoreHighlightModifiedTabs branch from 7300696 to bd1149d Compare April 3, 2023 13:38
Problematic implementation of `box-shadow` parameter.
Changed to `border` parameter.

Suggesting to change to use of the `border` parameter when
`box-shadow` specific properties are not necessary.

Signed-Off-By: FernandoAscencio <[email protected]>
@FernandoAscencio FernandoAscencio force-pushed the fa/restoreHighlightModifiedTabs branch from bd1149d to 9d49362 Compare April 3, 2023 13:57
@FernandoAscencio
Copy link
Contributor Author

FernandoAscencio commented Apr 3, 2023

@msujew
Thanks for the feedback.
I have solved the issue with a bit more CSS. However, it is restricted to one specific border thickness. With this solution, both for the normal top border highlight and the highlightModifiedTabs border highlight are restricted to their current thickness. As the tab.activeBorder and tab.unfocusedActiveBorder are both used to add the bottom border in High Contrast themes (as box-shadow originally and as border here), I doubt their thickness will change for the bottom. I am not sure we care much about the thickness change in the top border when we want to highlight modified tabs.

If you are interested I can do another PR with the solution involving box-shadow I previously did.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Alright, I'm fine with the proposed solution as well. Looks good to me 👍

@msujew msujew merged commit be8d785 into eclipse-theia:master Apr 4, 2023
@paul-marechal paul-marechal added this to the 1.37.0 milestone Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui/ux issues related to user interface / user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

workbench.editor.highlightModifiedTabs has not effect
3 participants