-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Persistent highlight is taller than transient highlight #3478
Comments
This is UTR for me on WinXP and on my non-retina Mac. Could it be a retina-only issue? |
I can reproduce it on my Mac. |
@RaymondLim is it retina or non-retina? Which version of OS X? |
It's retina. OS 10.8.2. Not sure it has to do with font or not since you won't see it with normal text size until you increase the font size multiple times. |
Just confirmed that this can be reproduced on Windows with very large font size. |
I was checking on Windows, and the problem is that the height of span elements inside the pre ones (the lines in CodeMirror) is bigger than the height of the actual pre elements when the the text-height is bigger than the default one. So then the search background that is applied over the span elements, is bigger than the selected search result highlight. The problem is that the line-height is increased at the same rate as the font-size, when it shouldn't. I could fix it by specifying the original line-height as 1.25em with and removing the line-height adjustment in the font-adjustment code. I'll post a fix for this using this solution. |
Reviewed. Low priority, to @TomMalbran since you've been looking into it already. Thanks! |
Setting to Fixed But Not Closed (FBNC) and assigning back to @iwehrman . Ian, confirm that bug is fixed and then close. |
Unfortunately I still see this problem at 2f2361e |
@iwehrman What OS are you using? Is this Brackets or Edge Code? |
I can't reproduce this on Windows anymore and I don't have a Mac to test it. Anyway, could it be a problem with having a line height in float values instead of integers for the pixel units? |
Yeah, I thought of that. I'll try rounding down. I can repro on my Mac, so I'll take a look. It's a retina display so the 1 pixel line is so thin I couldn't see it at max font-size until I took a screen shot and zoomed in :) |
@TomMalbran I'm seeing it with |
Maybe the line height needs to be bigger in Mac since the font is a bit bigger? Maybe adjusting the LINE_HEIGHT ratio constant could fix it? |
Wrong button, wanted to discard the comment since I saw @redmunds request that should fix it. |
FBNC back to @iwehrman (again) |
I cant reproduce this anymore on Windows. I tried with the font at 72px and checked it zoomed to find the 1px line, but there isn't one anymore. I am actually starting to see spaces between consecutive background highlights, which is expected with a line height bigger than what it should be. |
@iwehrman Does it happen for every text string, every time? If it only happens in certain cases, then please provide more details. Also, it appears that you're on a non-retina system, correct? What version of Mac? |
This was auto-closed. Re-open. |
@iwehrman I can no longer reproduce this one. Are you still seeing it? |
I still see it on b6c5880. Retina and non-retina screenshots below, which show highlight overlap all over the place. |
@iwehhrman I'm not seeing it on Win7 or Mac10.8 (both retina and non-retina). What OS are you on? Does it happen at every zoom level, or only certain ones? If you have any extensions installed, try disabling/uninstalling them. |
My system is 100% clean. I just rebuilt brackets-shell from master, disabled all my extensions, cleared cef_data and pulled again from master so I'm now at 3d4352a. I'm on the same computer and operating system I was on a week ago. Note, however, that my font size has been bumped up one notch using View > Increase Font Size. Sorry I neglected to mention that earlier 😬 |
I can reproduce on both Mac and Windows with this recipe:
Doesn't seem to matter which order steps 2 and 3 are done, but that's the only font size I have seen the problem at. |
I found out that there is a rounding problem, but different from the one we initial thought of. CodeMirror seems to be doing the rounding, if you increase the font size and check both the line height and the value returned by So @redmunds original fix at #3757 should solve this problem. I did checked that it actually fixed it. |
@TomMalbran That pull request used Math.ceil(). I pushed a fix that uses Math.round(), which should be more accurate and consistent with other rendering. Give #3849 a try and let me know how it works. |
Well since we use 1.3 as the line height ratio, Math.round seems to work. I tested it and I couldn't see the background highlight when increasing the font size once. |
FBNC to @iwehrman |
Yep, fixed. Closing. Nice work! |
The text was updated successfully, but these errors were encountered: