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

[decorators] Visible whitespace conflicts with before decorator attachment #11485

Closed
eamodio opened this issue Sep 3, 2016 · 12 comments
Closed
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded
Milestone

Comments

@eamodio
Copy link
Contributor

eamodio commented Sep 3, 2016

  • VSCode Version: Version 1.5.0-insider (1.5.0-insider) 1de4d83
  • OS Version: OS X El Capitan Version 10.11.6 (15G31)

I'm working on a new extension GitLens with a feature to provide "inline" blame support to the active text editor. I have it working quite well now, except for when Visible Whitespace is enabled. See the following:

It works quite well with whitespace hidden:

no-visible-whitespace

Unfortunately with Whitespace visible, not so much:

visible-whitespace

I've tracked it down to the inline width style that is added to the leading whitespace span. If that inline style is changed to min-width everything works, but I'm not sure on the ripple effect. Nor could I find exactly where that inline style is being added -- it looks like it starts in here. Another option is to remove inline-block from .monaco-editor .token.whitespace which also fixes the issue.

I am currently working on this in a branch, but you can find the code that sets up the decorations here.

Any help on this would be greatly appreciated.

Thanks,
Eric Amodio

@alexdima
Copy link
Member

alexdima commented Sep 5, 2016

We render the dot and the arrow inline in the case of whitespace. For some fonts, those symbols are not monospace, which was causing indentation to be miss-aligned, hence the decision to set their width. I am not sure if we tested the inline decorations around whitespace.

@eamodio
Copy link
Contributor Author

eamodio commented Sep 5, 2016

@alexandrudima that makes sense. Is there anyway around this for me? As an extension my control over the CSS of the before decoration is quite limited. And while setting min-width vs width would fix this issue there might be other unforeseen issues.

@aeschli
Copy link
Contributor

aeschli commented Sep 7, 2016

The bug is related to the css rules we use to show the before and after decorations. Unfortunatly not an easy fix.

@eamodio Just a comment on your use case. I wonder if you the before annotations are the right thing to use here. As you know, empty lines can not have such decorations and we also don't offer any mouse click listener of decorators (maybe you found a workaround?)

@aeschli aeschli added the bug Issue identified by VS Code Team member as probable bug label Sep 7, 2016
@aeschli aeschli added this to the Backlog milestone Sep 7, 2016
@aeschli aeschli changed the title Visible whitespace conflicts with before decorator attachment [decorators] Visible whitespace conflicts with before decorator attachment Sep 7, 2016
@eamodio
Copy link
Contributor Author

eamodio commented Sep 7, 2016

@aeschli Thanks for the response. Yeah, I agree that the before annotations aren't really the right thing you use here, but since there isn't any control over the gutter -- I don't think I really have another option.

Other than this issue (and the much more minor issue with blank lines), it has worked out well enough for what I'm trying to do (granted if I had more control over it and events that would be even better -- though not really required in this case). You can see it in action (here)[https://marketplace.visualstudio.com/items?itemName=eamodio.gitlens].

I currently have "hacked" around the issue by executing the command to toggle on and off the visible whitespace, which works ok most of the time, but that command seems to be workspace wide when executed when the active editor is a "normal" editor. But when it is a diff, the command seems to be local to the specific diff pane. So it certainly isn't ideal -- especially since I can't actually read the current state of the whitespace (other than via the configuration, which reports the workspace state, not the diff state).

@eamodio
Copy link
Contributor Author

eamodio commented Feb 6, 2017

@aeschli It looks like this issue has been fixed. Is that the case or am I getting lucky somehow? :)

@alexdima
Copy link
Member

alexdima commented Feb 6, 2017

@eamodio It is just luck. I've pushed a while ago an unrelated change to optimize the editor rendering in case of using a monospace font. Parts of that optimization is that whitespace width is not set when rendering whitespace and the font is monospace. I'm guessing if the font is changed to Arial the issue resurfaces.

@eamodio
Copy link
Contributor Author

eamodio commented Feb 6, 2017

@alexandrudima Ah, thanks for the info (Glad I left the code in GitLens to toggle whitespace, if need be).

@eamodio
Copy link
Contributor Author

eamodio commented Feb 18, 2017

@alexandrudima Is there way that an extension could tell if a monospace font was in use? I see that vscode does a bunch of work to figure that out, but I don't think it is exposed anywhere -- is it?

@eamodio
Copy link
Contributor Author

eamodio commented Apr 25, 2017

@alexandrudima is there any way to get this bumped up or some way I could help with this (I honestly have no idea where to start on this one) -- it causes significant grief for GitLens ;)

@alexdima
Copy link
Member

alexdima commented Apr 27, 2017

The real fix for this is to us to move away from CSS before/after selectors ::before and ::after for the implementation of inline decorations. It is not an easy change, right now these elements are transparent to JavaScript (they get inserted via the CSS selectors), so the current implementation is "dumping" a class name on a span node.

The real fix would be for the view to become aware of these elements and generate them by ourselves when painting lines and also take them into account when doing things like hit testing for mouse movement, etc.

@eamodio
Copy link
Contributor Author

eamodio commented Apr 27, 2017

Ah -- that makes complete sense -- thanks for the write up.

Until that happens, do you think there is any path to a hack with the current implementation -- as it works fine for monospace fonts? -- I'm assuming not, but just wanted to check ;)

@eamodio
Copy link
Contributor Author

eamodio commented Jul 5, 2017

WAHOO! Can't wait to try this out! THANK YOU!

@roblourens roblourens added the verified Verification succeeded label Aug 2, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

5 participants