Fix icon text fit

[kkaefer]

We're adding a 1 pixel border around all icons when adding them to the image atlas texture to avoid having neighboring icons bleed into the current image when using linear texture interpolation. When stretching icons to fit the text's dimensions with icon-text-fit, however, we didn't account for this correctly: We didn't add the border in this case, resulting in stretched icons that didn't quite cover the whole text. When the difference between the icon's original size and the stretched size was small, this wasn't really noticeable, especially in cases where we were using nearest neighbor texture interpolation instead of linear interpolation. When accounting for the 1 pixel border, have to to take into account that there's no longer a 1:1 mapping between vertex units and pixel units, and expand the vertices adequately.

Here's an example:

Before	After

This example uses a really small icon so that the discrepancy between the true size and the scaled size is big, which leads to a large error.

Native patch is forthcoming.

[kkaefer]

This PR changes the way icons are aligned with text when the *-anchor doesn't match. Here are all combinations before the PR:

and with this PR applied:

As you can see, the previous code essentially ignored the icon-anchor value when at least one stretch direction was set (i.e. icon-text-fit != none), while the new PR respects the icon's position in the direction that wasn't stretched.

This makes me believe that we should better define how icon-text-fit is supposed to work:

• Should icon-text-fit only affect the sizing of the icon, or should it also affect the positioning? At the moment, it's a hybrid of both.
• Does this also apply to icons/text that is moved relative to the anchor point with icon/text-offset?
• Are there valid use cases for fitting an icon to the text size, but not also positioning it with the text?
• When applying a icon-text-fit-padding, should we respect the padding for the dimension that is not stretched in case of width/height?

/cc @mapbox/design @mapbox/studio

[tristen]

Are there valid use cases for fitting an icon to the text size, but not also positioning it with the text?

I don't think there are valid use cases for that.

---

@kkaefer My 2c would be this: When icon-text-fit is set, icon position properties should no longer work: it should now be controlled by text positioning.

[asheemmamoowala]

@kkaefer Does this also address the collision issue described in #8722 ?

cc @zmiao

[alexshalamov]

There is quite a big difference in test/integration/render-tests/text-variable-anchor/all-anchors-icon-text-fit/expected.png is it caused by incorrect icon collision boxes?

[kkaefer]

@asheemmamoowala it didn't initially, but I changed the behavior so that the text-fitting happens earlier in the process and is now respected in the collision boxes it generates.

@tristen I made an example page that allows playing with the various values and how they're affecting positioning:

• Unfixed/current version
• Fixed/new version

I increased the size so that it's easier to see. In Firefox and Chrome, antialiasing for the resized canvas is disabled (Safari doesn't have support for that).

A few other things that are fixed:

• The icon-offset is now applied after fitting the icon to the text.
• The icon collision box is now correctly reflecting the actual icon position, after fitting + translation, and padding

There are also some changes that I'm not sure about:

• When icon-text-fit is height, we're not applying the left and right padding; only the bottom and top padding. Same applies to width and top/bottom padding. Should this behavior be reverted? I.e. should we always apply the icon-text-fit-padding in both axes regardless of whether the icon is fitted on that axis?
• icon-anchor is now without function if icon-text-fit is set to width/height/both and the icon is always centered on the text. We could reuse icon-anchor in this case to describe where the icon should be anchored to within the text frame. However, it also seems a bit weird to reuse the same name for this slightly different feature, and maybe use icon-text-fit-anchor for that instead if we decide to implement it.

Using icon-size with non-center text-align as well as a icon-text-fitting produces a kind of parallax effect and the icon isn't centered anymore on the text. This is unchanged compared to the existing state. There's no easy way to fix this because the scale anchor would have to be relative to the text's position, which is already relative to the object's anchor position. Should we disable icon-size when icon-text-fit is set?

[tristen]

Oh awesome @kkaefer "Fixed/new version" is 👨‍🍳 💋

[asheemmamoowala]

When icon-text-fit is height, we're not applying the left and right padding; only the bottom and top padding. Same applies to width and top/bottom padding. Should this behavior be reverted? I.e. should we always apply the icon-text-fit-padding in both axes regardless of whether the icon is fitted on that axis?

This seems like reasonable behavior to me, and provides decent outcomes, but I wonder if there is enough of a use-case to add support and documentation for this nuance. icon-text-fit is not data-driven yet, so these style parameters are fixed at design time. It would make more sense to add this if/when icon-text-fit is data driven.

icon-anchor is now without function if icon-text-fit is set to width/height/both and the icon is always centered on the text. We could reuse icon-anchor in this case to describe where the icon should be anchored to within the text frame. However, it also seems a bit weird to reuse the same name for this slightly different feature, and maybe use icon-text-fit-anchor for that instead if we decide to implement it.

Using icon-size with non-center text-align as well as a icon-text-fitting produces a kind of parallax effect and the icon isn't centered anymore on the text. This is unchanged compared to the existing state. There's no easy way to fix this because the scale anchor would have to be relative to the text's position, which is already relative to the object's anchor position. Should we disable icon-size when icon-text-fit is set?

👍 to disabling icon-size and icon-anchor when using icon-text-fit.

[kkaefer]

For the sake of getting this merged let's defer dealing with the icon-size issue when icon-text-fit is set to another PR.

[alexshalamov]

looks great!

One question, not directly related to changes in this PR. In some of the tests, for instance, both-text-anchor-1x-image-2x-screen, text is not center-aligned to the anchor, is it a bug?

[kkaefer]

One question, not directly related to changes in this PR. In some of the tests, for instance, both-text-anchor-1x-image-2x-screen, text is not center-aligned to the anchor, is it a bug?

Can you say more? I believe that this test image is correct.

[alexshalamov]

Can you say more? I believe that this test image is correct.

Visually, looks like text should be shifted few pixels to the left. Letter 't' is touching right edge of a stretched icon, while there is quite a big gap in front if 'Ü'

[kkaefer]

Hm, I believe that this can be attributed to the glyph metrics of this particular letter. For comparison, here's a rendering with a different letter which goes all the way to the edge:

[asheemmamoowala]

There are ~9 rendered tests added here with the same geojson source data. Could those be moved into a separate fixture?

Could you also update the style spec doc for text-icon-fit to indicate that it disables icon-size and icon-anchor.

[kkaefer]

I decided to move this to the next release since we'll have a few other fixes and behavior changes for icon-text-fit as well in there.

[kkaefer]

This PR sizes and positions the collision boxes correctly, except when using text-variable-anchor. In that case, the size is correct, but the position isn't. @ansis, how can we ensure that icons shifted by the variable text-anchor placement is reflected in the collision index as well?

[ansis]

I pushed two commits to fix-icon-text-fit-variable that fix collisions for icon-text-fit when combined with variable text anchors. The first commit just shifts the icon collision box to match the chosen text position. From what I can understand this is what -native does. This doesn't work ideally in the case the text has no collisions but the icon-text-fit icon does. In this case the label might be hidden instead of shown in a different position. The second commit tries to avoid this by testing for icon collisions while picking the text anchor. That may be an improvement.

Both commits fail the icon-text-fit/text-variable-anchor render tests. I haven't investigated whether this are good or bad differences.

[kkaefer]

Thanks @ansis! I only picked the first commit because this puts us at par with GL Native. Let's land the second patch in a separate PR.

[asheemmamoowala]

I only picked the first commit because this puts us at par with GL Native. Let's land the second patch in a separate PR.

@kkaefer is there a ticket or PR for the second patch mentioned here?

[kkaefer]

Yes, it's at #8803