align gutter png icon position with line number

[InspiredGuy]

Issue #, if available:
#5380

Description of changes:
Moving the background which sets the gutter png icons from .ace_gutter-cell.ace_error to .ace_gutter-cell.ace_error .ace_icon. It's the same container used for displaying annotations in folded block.

Initially I've tested a naive fix for this by moving the background position to top instead of center (same positioning would be achived with background-position: 2px -1px; instead of background-position: 2px center;). It looked OK on default font size, but with font size changing it did not keep up the alignment with line numbers (see screenshot below). Solution implemented in this PR keeps the icons aligned with line numbers regardless of font size.

To test this open https://raw.githack.com/ajaxorg/ace/fix-gutter-icon-position/kitchen-sink.html, set the soft wrap to 40 and type long enough and wrong enough to see annotations in a multiline row,

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Pull Request Checklist:

• No backwards incompatible changes were made to Ace's public interface which is defined through the main typings file (ace.d.ts) and its references:
  • https://github.com/ajaxorg/ace/blob/master/ace.d.ts
  • https://github.com/ajaxorg/ace/blob/master/ace-modes.d.ts
  • https://github.com/ajaxorg/ace/blob/master/ace-extensions.d.ts

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.89%. Comparing base (b7495e1) to head (588fe48).
Report is 133 commits behind head on master.

Additional details and impacted files

@@ Coverage Diff @@ ## master #5611 +/- ## ======================================= Coverage 86.89% 86.89% ======================================= Files 594 594 Lines 43143 43158 +15 Branches 7150 7150 ======================================= + Hits 37489 37504 +15  Misses 5654 5654 

Flag	Coverage Δ
unittests	86.89% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[InspiredGuy]

@nightwing @akoreman let me know if you have any concerns about moving the background to another element. I've checked the codebase but it doesn't seem the target element is used for anything except for showing errors in folded block (which is not affected by this change).

[akoreman]

I think the one concern would be what would happen if people currently have a custom theme theme set the background image to .ace_gutter-cell.ace_error? Would this change break them by displaying both background images on top of each other?

On the other hand, this is a bullet we might need to bite at some point and accept that for these rare use cases people will have to make a change, but in that case, would it be better to just fully deprecate the PNG icons and fully go for the new SVG ones?

[InspiredGuy]

I think the one concern would be what would happen if people currently have a custom theme theme set the background image to .ace_gutter-cell.ace_error? Would this change break them by displaying both background images on top of each other?

Yeah, it's not like their app won't run but this scenario you've described is certainly possible. It might be even worse since it will be implicit.

On the other hand, this is a bullet we might need to bite at some point and accept that for these rare use cases people will have to make a change, but in that case, would it be better to just fully deprecate the PNG icons and fully go for the new SVG ones?

This would be breaking on the interface level as we'd need to deal with the API property we have. If we remove it it's kind of a breaking change, and if we just ignore the passed value it's also kind of a breaking change. It might be wise to save such moves until the next major version :)

[InspiredGuy]

I've updated the PR to calculate the background position based on the font size and icon size in the currently used container. Value for line height is 1.2, which is the same as normal (see docs). Icon size is 16x16 implicitly. It can be tested here - https://raw.githack.com/ajaxorg/ace/fix-gutter-icon-position/kitchen-sink.html

Not the prettiest solution, and if we think it's too risky I'm OK to discard it completely, this issue is not super important.

FYI @akoreman

[InspiredGuy]

@nightwing I've checked the codebase and it seems like css variables are not used anywhere yet. Is there a reason for this? They seem well-supported across browsers.