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

[EuiIcon] Icon asset paths/content off center from viewbox #7723

Closed
Tracked by #182611
nickofthyme opened this issue Apr 30, 2024 · 8 comments
Closed
Tracked by #182611

[EuiIcon] Icon asset paths/content off center from viewbox #7723

nickofthyme opened this issue Apr 30, 2024 · 8 comments

Comments

@nickofthyme
Copy link
Contributor

nickofthyme commented Apr 30, 2024

Is your feature request related to a problem? Please describe.

Looking at an alignment issue in charts I noticed the icon was just slightly offset from the chart LineAnnotation.

image

Then looking at the raw eui svg, I could clearly see an offset to the left.

Comparing this to other public libraries like Material Symbols, I see the icons correctly centered.

image

I'm not sure how many of the icons are affected but this is quite noticeable on icons which are horizontally symmetrical in design.


I'm no svg expert here and could be missing something but thought I'd share.

Describe the solution you'd like
Go through all icons and center them as needed.

Desired timeline
Whenever

cc: @MichaelMarcialis

@nickofthyme nickofthyme changed the title [EuiIcon] Icon assets paths off center from viewbox [EuiIcon] Icon asset paths/content off center from viewbox Apr 30, 2024
@MichaelMarcialis
Copy link
Contributor

@nickofthyme: I'm guessing the 1px offset for this icon was used to ensure the 1px line portion stays perfected on the pixel grid. As the viewbox for EUI icons are 16x16px, this requires an offset of 1px. The reason we try to stick to the pixel grid as often as possible is because subpixels rendered on monitors that aren't a high pixel density end up looking blurry.

As for this particular icon, we can either:

  1. Center align the icon regardless of not being on the pixel grid (and accept the potential blurriness on low pixel density displays).
  2. Or tweak the icon so that the 1px stroke is 2px for perfect centering on a 16x16px viewbox.

I'll explore both later today and let you know what I'd recommend.

@MichaelMarcialis
Copy link
Contributor

MichaelMarcialis commented May 6, 2024

@nickofthyme: Does something like this work for your needs? I made some adjustments to allow the icon to be perfectly center in the viewbox while still adhearing to the pixel grid. If this works for you, I'll make a PR to update EUI with it.

CleanShot 2024-05-06 at 15 03 50

Also, we seeing as your Material icon example used a more traditional flat-top push pin, I just wanted to note that we also have pin and pinFilled icons in EUI, both of which have the same 1px offset. Do you need those updated as well? Or was this request strictly for the annotation icon?

@nickofthyme
Copy link
Contributor Author

Oh gotcha, that makes more sense why it was done that way.

I don't have any need for the pin specifically, I just noticed that the it along with other icons were off center, the Material pin was just an example of a centered icon. I wonder if it's worth expanding the icons to a 24px x 24px grid to allow greater flexibility with centering without expanding the lines or splitting grid pixels just to center symmetrical icons.

@nickofthyme
Copy link
Contributor Author

nickofthyme commented May 6, 2024

A good number of the symmetrical icons are shifted in this way, just listed a few more below. This is not too noticeable, depending on the use case, more noticeable when icons are aligned vertically to something or has some type of border around it. But in any case, ideally they should be centered, no??

Icon Preview
error image
home image
globe image
share image
pinFilled image

@MichaelMarcialis
Copy link
Contributor

A good number of the symmetrical icons are shifted in this way, just listed a few more below. This is not too noticeable, depending on the use case, more noticeable when icons are aligned vertically to something or has some type of border around it. But in any case, ideally they should be centered, no??

Ideally, yes, they would always be perfectly vertically and horizontally centered. In reality though, if we merely centered them, those 1px lines within each of the icons you reference would become blurry on low pixel density displays (i.e. non-retina). Here's an example on my lower pixel density monitor (~110ppi) at 100% scale. Notice that the perfectly centered icon on the right is a bit less crisp and blurrier than the one on the left with the 1px offset.

CleanShot 2024-05-06 at 16 29 04

I don't have any need for the pin specifically, I just noticed that the it along with other icons were off center, the Material pin was just an example of a centered icon. I wonder if it's worth expanding the icons to a 24px x 24px grid to allow greater flexibility with centering without expanding the lines or splitting grid pixels just to center symmetrical icons.

Unfortunately, we would still likely encounter this problem even if we increased the source SVG viewboxes to 24x24px due to the nature of attempting to center a 1px element on an plane that has an even-numbered width. Additionally, these 24x24px icons would still be rendered at a scaled down size of 16x16px or 12x12px across many areas of Kibana. This downscaling could create even more instances where subpixels would be antialiased and appear blurry on these lower density displays.

Ultimately, I think we need to ask ourselves which is the lesser evil? Is it better for us to have perfectly centered icons and risk them being a bit blurrier on the majority of displays? Or is it better to keep the icons appearing as crisp as possible with the occasional offset as needed? Historically, we've opted for the latter, but happy to discuss further if there are differing opinions.

@tkajtoch
Copy link
Member

tkajtoch commented May 9, 2024

The way I see it, the only real solution is to redraw all icons that don't take the whole 16x16 grid or take an odd number of pixels on any axis, so they can't be centered using the remaining number of pixels without introducing sub-pixel rendering.

The quality of sub-pixel rendering depends on pixel density, display sub-pixel layout, display rotation, video signal compression, operating system, browser, and even drivers. We don't control any of these, but we should always try to render the best view possible.

I also want to note here that sub-pixel rendering doesn't only apply to our icons. Some of our font sizes also fall into this category—our rem units resolve to values like 16.0006px and 21.9996px. While the difference is really small and not detectable on smaller text sizes, it still exists.

It's a lot of work for a relatively tiny difference.

Just for fun: nearest pixel rounding diffs

Original image:
original

Chrome @ MacOS (3456 × 2234, 226ppi) diff:
mac_diff

Chrome @ Windows (3840 x 2160, 137ppi) diff:
windows_diff

@MichaelMarcialis
Copy link
Contributor

Thanks for those additional points, @tkajtoch. I was actually just recently wondering about our EUI font size values.

All that said @nickofthyme, do you feel it's still worth it for us to update the annotation icon with the changes I shared above? If so, let me know and I can cut an EUI PR for the change.

@nickofthyme
Copy link
Contributor Author

All that said @nickofthyme, do you feel it's still worth it for us to update the annotation icon with the changes I shared above?

No I just wanted to make you all are aware of it, if we can fix/consider it progressively as we add new icons or update old ones.

As for the sub-pixel blurring, those are really good points, though it seems like the vast majority of kibana users are using HD screens. So I personally would favor centering icons over avoiding sub-pixel blurring, possibly in a 24px x 24px grid.

Resolution % of users
1920 x 1080 40%
1536 x 864 11%
2560 x 1440 11%
1280 x 720 8%
1366 x 768 6%

Anyways I'll go ahead and close this issue for now.

@cee-chen cee-chen closed this as not planned Won't fix, can't repro, duplicate, stale May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants