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

Remove ltr-specific CSS in favor of transparent/non-opaque. #1396

Closed
wants to merge 4 commits into from

Conversation

FGasper
Copy link
Contributor

@FGasper FGasper commented Apr 19, 2018

Issue 1330: This is a proper fix after the original (b27d5bb) failed
to fix the issue.

Issue 1330: This is a proper fix after the original (b27d5bb) failed
to fix the issue.
@FGasper
Copy link
Contributor Author

FGasper commented Apr 19, 2018

#1330 @Tyriar

@@ -66,7 +66,8 @@
*/
position: absolute;
opacity: 0;
left: -9999em;
color: transparent;
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the element still there though unless we reposition and it could intercept mouse presses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It’s got a -10 z-index … that would seem to be intended to prevent that?

Copy link
Member

Choose a reason for hiding this comment

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

It seems like by suspicion was correct, at least for the cursor type. While running in tmux the cursor should be the default pointer but mousing over the textarea location shows a text cursor.

image

The mouse event appears to be redirected correctly but we should fix the hover issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps a pointer-events: none in the textarea’s CSS?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, as long as you can hover without the cursor changing and click it and the event is directed to be handled by the terminal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tyriar I actually see the text cursor for the entire xterm area, even on master.

I did go ahead and push up the pointer-events: none change. I’m not sure how to reproduce the change you were seeing, but when you have a spare minute give it a look.

Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

@FGasper are you running tmux in mouse mode?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like that fixed it 👍

@Tyriar Tyriar added this to the 3.4.0 milestone Apr 23, 2018
@Tyriar Tyriar self-assigned this Apr 23, 2018
@@ -61,12 +61,16 @@

.xterm .xterm-helper-textarea {
/*
* HACK: to fix IE's blinking cursor
* Move textarea out of the screen to the far left, so that the cursor is not visible.
* This used to fix IE's blinking cursor by moving the textarea
Copy link
Member

Choose a reason for hiding this comment

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

Did you check if it still works in IE11? I can test this if you don't have access to a machine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, of course, it blinks … lemme play with it a bit here …

@Tyriar Tyriar modified the milestones: 3.4.0, 3.5.0 May 1, 2018
@Tyriar Tyriar added the work-in-progress Do not merge label May 15, 2018
@Tyriar Tyriar removed this from the 3.5.0 milestone Jun 21, 2018
@Tyriar
Copy link
Member

Tyriar commented Sep 8, 2018

Looks like the master branch already has this fix (as it would fail lint now).

@Tyriar Tyriar closed this Sep 8, 2018
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.

2 participants