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

Format right-to-left and bi-di text in OSM tag values #4333

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

mapmeld
Copy link
Contributor

@mapmeld mapmeld commented Nov 8, 2023

The change fixes an issue where right-to-left language strings can be rendered incorrectly in the Tags table of Map Data or History views

Sample bug in text rendering

I noticed this street label in Erbil with mismatched parentheses. This turned out to be an error in the name string:

Screen Shot 2023-11-08 at 11 01 08 AM

When reviewing the name in the Map Data (or now, History view), it looks like the name is wrapped in parentheses. With a wider box or shorter string, the parentheses appear to wrap one word (مشکۆ) . What we're really seeing is the 0th character of the name string , ( , displayed on the left end of this line.

Screen Shot 2023-11-08 at 11 06 06 AM Screen Shot 2023-11-08 at 11 21 52 AM

Corrected

In an updated version with dir="auto", the string renders like it did in the tiles. This makes the misplaced and unpaired parenthesis visible in the web view.

Screen Shot 2023-11-08 at 11 22 16 AM

CSS has a direction property, but it can only be set to ltr or rtl, not auto

I've edited the name of the original way, but the issue can still be seen in the History tab.

@gravitystorm
Copy link
Collaborator

Hi @mapmeld thanks for your pull request.

This PR addresses one small part of #3428 (and see also #3429 which mostly implements the rest, but is not yet merged).

Have you considered also setting dir=auto for keys? I know the convention in OSM is to use (mostly) English-language keys, but if we are establishing that all user-generated-content should be dir="auto" as part of the wider work, then perhaps we should consider setting it for keys here too. What do you think?

In any case, there has been other work on this file since your PR was created, so it needs to be rebased before we can merge it. Can you take care of that too please?

@gravitystorm gravitystorm added the changes requested Changes are needed before the PR will be merged or reviewed again label Nov 15, 2023
@mapmeld
Copy link
Contributor Author

mapmeld commented Nov 15, 2023

@gravitystorm thanks - I rebased and have made both key and val have dir="auto". I don't know of examples where the key is in an RTL language, but I agree it's alright to add it on.

@gravitystorm gravitystorm merged commit ba2b973 into openstreetmap:master Nov 15, 2023
11 checks passed
@gravitystorm
Copy link
Collaborator

Thanks @mapmeld , merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested Changes are needed before the PR will be merged or reviewed again
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants