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

text-radial-offset does not account for icon size or bounding boxes #8598

Closed
chloekraw opened this issue Aug 5, 2019 · 11 comments · Fixed by #8642
Closed

text-radial-offset does not account for icon size or bounding boxes #8598

chloekraw opened this issue Aug 5, 2019 · 11 comments · Fixed by #8642
Assignees
Labels

Comments

@chloekraw
Copy link
Contributor

Stub ticket to capture customer feedback:

text-radial-offset does not take into account symbol icon dimensions. Text with text-radial-offset may overlap wide or horizontally stretched icons with a "*-left"/"*-right" text-variable-anchor position.

text_right_shift_broken

cc @pozdnyakov @alexshalamov

@chloekraw chloekraw changed the title text-radial-offset does not take into account icon bounding boxes text-radial-offset does not account for icon size or bounding boxes Aug 5, 2019
@chloekraw chloekraw added this to the release-r milestone Aug 5, 2019
@pozdnyakov
Copy link
Contributor

@chloekraw thanks for filing the issue!

In order to solve it, we could introduce two component text-radial-offset (i.e. offset along X axis and offset along Y axis ). So, instead of a circle around the anchor origin we'll have an ellipse.

Text radial offset shall be defined following:
"text-radial-offset"?: DataDrivenPropertyValueSpecification<Array<number>>

  • if text-radial-offset is initialized with a number (to keep the existing styles working) or with an array of one element, we'll use the circle offset (same way we do now).

actual

  • if text-radial-offset is initialized with an array of two elements, these two numbers will stand for the radius on the x and y axes of the ellipse offset.

actual

@chloekraw
Copy link
Contributor Author

chloekraw commented Aug 14, 2019

@ansis made a good point on the PR:

Is placing around an ellipse what we're looking for? It looks like placing around a rectangle might work better for #8598

I agree, it's more likely that designers will implement rectangular rather than elliptical icons, and an elliptical offset won't necessarily provide the optimal text offset against all rectangular icons.

@mapbox/map-design-team let us know if you have feedback on this.

@pozdnyakov
Copy link
Contributor

@chloekraw @ansis ellipse corresponds better to radial offset property name as it has radius. In this perspective it looks like natural extension of the existing API.

For rectangular offset, we could probably use the existing text-offset. Currently, it is disabled by both text-radial-offset and text-variable-anchor. We could allow it with text-variable-anchor and specify dedicated rules for this case, e.g. assuming we got [x, y] the offset will be calculated based on the text-variable-anchor values as following :

  • "left": [-x, 0]
  • "right": [x, 0]
  • "top": [0, -y]
  • "bottom": [0, y]
  • "top-left": [-x, -y]
  • "top-right": [x, -y]
  • "bottom-left": [-x, y]
  • "bottom-right": [x, y]

We could allow only positive x and y values for simplicity.

WDYT?

@chloekraw
Copy link
Contributor Author

chloekraw commented Aug 15, 2019

The more I step back and contemplate this issue, the more I think that this is an unnecessary problem we created for ourselves by allowing corner placements as options in text-variable-anchor in the first place.

Whether icons are wide or tall, circular, square, or rectangular in shape or precisely tailored to the outline of a landmark, I don't think that text labels placed using the "top-left", "top-right", "bottom-left", "bottom-right" placement options will look good. For most utility maps, I can't imagine a text/icon design in which you'd want the text to start at the corner of an icon on a map.

In fact, I think it would actually engender confusion if you mixed top/left/right/bottom placements with corner placements. Imagine icon A is placed to the left of icon B, icon A contains fairly long text placed at "bottom-right", and icon B doesn't contain text. Now you have text that's awkwardly positioned between the two icons or text that actually could look like it belongs more to icon B because it's in icon B's "bottom" placement.

I can imagine a hypothetical avant garde map design that used all corner placements for text labels, emanating a unique artistic appeal. I can also imagine that there could be use case for corner placements on a map layer in which the icons aren't POIs and the corresponding text isn't POI names. But I think that we should have held off on supporting corner placements in the first iteration of text-variable-anchor and seen whether broad demand for this enhancement would emerge.

I checked Google Maps and Apple Maps, and neither use corner placements for text labels. There are a few instances where "bottom" placement was combined with "text-justify": "right"/"left". This made me think that using the "bottom-right" placement in conjunction with "right" text justification would alleviate some of the design challenges I highlighted above. However: I think that (1) the "bottom" placement option fulfills this design need, and (2) until we add support for inputting an array of enums to text-justify and design an intuitive way for designers to combine text-justify options with text-variable-anchor options, this is pretty much a moot point.

I make this point not to suggest that we remove support for corner placements retroactively, but to propose that we close this issue and punt on implementing changes to text-radial-offset until we see customer reports of this problem. text-radial-offset works well as-is for non-corner placements. Let's make sure it's actually broken before we fix it and the problem we're intending to fix is a real problem.

EDIT: I should add that specifically, the problem we're seeing in prototype testing is that text-radial-offset doesn't work well for providing the optimal text offset when corner placements are mixed with non-corner placements in the same symbol layer. If you were using all corner placements, you could still identify an optimal value for text-radial-offset.

Anyone have alternate thoughts on this issue?

@mapbox/gl-js @mapbox/map-design-team @ajashton @ian29 @mzdraper @amyleew @philogb @brsbl

@pozdnyakov
Copy link
Contributor

I'd like to mention that the issue is not exactly about corner anchors, if the designer uses only top, bottom, left and right two component offset still looks useful, pls consider

current solution:
one

vs proposed solution
two

@chloekraw
Copy link
Contributor Author

Gotcha, thanks. @pozdnyakov is that prototype with the elliptical pr you already have open or with a rectangular proposal?

@pozdnyakov
Copy link
Contributor

is that prototype with the elliptical pr you already have open or with a rectangular proposal?

elliptical, but the rectangular would look the same, just API will be different.

@pozdnyakov
Copy link
Contributor

IMO rectangular proposal is better than elliptical proposal as it provides backward compatibility for the existing styles and applications without extra efforts.
If there are no objections, I could shortly draft a proto for both mapbox-gl-native and mapbox-gl-js.

@chloekraw
Copy link
Contributor Author

@pozdnyakov I think that makes sense! I haven’t totally thought through the backwards compatibility question, but if it is backwards compatible in all cases, then I think that’s how we should have supported offsetting text with variable label placement. Though, that said, I wasn’t a part of those design discussions and I think feedback from map design on this matter will be very valuable.

Personally I have no objections to a prototype, especially if it’s not too much work. Conceptually I think it works to have text-variable-anchor function with both text-offset and text-radial-offset, but the two offset properties disable each other. Is that possible, to make the presence of whichever offset property appears first disable the other?

@pozdnyakov
Copy link
Contributor

Personally I have no objections to a prototype, especially if it’s not too much work. Conceptually I think it works to have text-variable-anchor function with both text-offset and text-radial-offset, but the two offset properties disable each other. Is that possible, to make the presence of whichever offset property appears first disable the other?

I would keep text-offset and text-radial-offset mutually exclusive, i.e. if text-radial-offset is defined - text-offset gets ignored (just same as now).

@pozdnyakov
Copy link
Contributor

Here is the "rectangular" approach prototype #8642

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants