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

Draw selection between background and foreground #776

Closed
wants to merge 5 commits into from
Closed

Draw selection between background and foreground #776

wants to merge 5 commits into from

Conversation

mofux
Copy link
Contributor

@mofux mofux commented Jul 11, 2017

This is an attempt to have the selection always underneath the foreground, but above the background, see #720.

selection

How it works

If there is a background color involved (and only in that case), an additional <span class="xterm-bg ..."></span> is added to the span that contains the characters. This span in positioned absolutely and fills the space of the parent span. It has a negative z-index to get below the parent span's text.

To have the selection between background and foreground, I had the move the .xterm-selection container inside the .xterm-rows container. Although this works great, it doesn't belong there semantically 🤧.

As the code changes are very small, this could be a temporary solution to #720 before a more efficient solution is introduced.

Update
I have found a much better way that will have no performance impact if selection is inactive, and only minor performance impact if selection is active. Instead of drawing the background in a separate span all the time, we create a :before pseudo element if selection is active (.terminal.selection-active). The :before element will fill the whole space of the span, draw the background and position itself underneath the text. I had to extend the xterm-bg-... css rules for this change.

Notice: Once truecolor support is implemented, this method will not work any longer. For truecolor we will likely have to create a real element for the background color, but for the time being this should get the job done. 🤓

@coveralls
Copy link

coveralls commented Jul 11, 2017

Coverage Status

Coverage remained the same at 69.444% when pulling d6bd943 on mofux:master into 5f817e8 on sourcelair:master.

@mofux mofux changed the title Selection between background and foreground Draw selection between background and foreground Jul 11, 2017
@mofux
Copy link
Contributor Author

mofux commented Jul 11, 2017

Update
I have found a much better way that will have no performance impact if selection is inactive, and only minor performance impact if selection is active. Instead of drawing the background in a separate span all the time, we create a :before pseudo element if selection is active (.terminal.selection-active). The :before element will fill the whole space of the span, draw the background and position itself underneath the text. I had to extend the xterm-bg-... css rules for this change.

Notice: Once truecolor support is implemented, this method will not work any longer. For truecolor we will likely have to create a real element for the background color, but for the time being this should get the job done. 🤓

@coveralls
Copy link

coveralls commented Jul 11, 2017

Coverage Status

Coverage decreased (-0.03%) to 69.416% when pulling f298b8f on mofux:master into 5f817e8 on sourcelair:master.

@Tyriar
Copy link
Member

Tyriar commented Jul 11, 2017

Notice: Once truecolor support is implemented, this method will not work any longer.

Truecolor #756 will hopefully go into either the next release or the one after, so we probably shouldn't move forward with this only to remove it before it's released or regress in the future.

@coveralls
Copy link

coveralls commented Jul 11, 2017

Coverage Status

Coverage decreased (-0.03%) to 69.416% when pulling e36dfb4 on mofux:master into 5f817e8 on sourcelair:master.

@mofux
Copy link
Contributor Author

mofux commented Jul 11, 2017

@Tyriar 👍 I'd like to keep this PR open and try to refine it once truecolor support has shipped.

@Tyriar
Copy link
Member

Tyriar commented Jul 11, 2017

@mofux sounds good. While you're thinking about this, try think of ways that we could support selection inverting as well (as an option). I've always thought it was a really great feature because it looks cool and does a good job of ensuring the text readable #692

@Tyriar Tyriar self-requested a review July 11, 2017 17:39
@Tyriar Tyriar added the work-in-progress Do not merge label Jul 11, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 69.388% when pulling 9e54e7c on mofux:master into 01453e9 on sourcelair:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 69.388% when pulling 9e54e7c on mofux:master into 01453e9 on sourcelair:master.

@mofux
Copy link
Contributor Author

mofux commented Sep 7, 2017

I'm closing this as we will get this in using the new canvas renderer.

@mofux mofux closed this Sep 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work-in-progress Do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants