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

Fix textAlign start/end for placement: line #14932

Merged
merged 2 commits into from
Aug 3, 2023

Conversation

ahocevar
Copy link
Member

Fixes #14930

@ahocevar ahocevar marked this pull request as draft July 21, 2023 09:43
@github-actions
Copy link

📦 Preview the website for this branch here: https://deploy-preview-14932--ol-site.netlify.app/.

@ahocevar ahocevar marked this pull request as ready for review July 21, 2023 10:13
Copy link
Contributor

@jahow jahow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change in this PR looks good but I'm confused because the image currently on main seems correct: "small text" is at the start of the line and "negative offset X" is at the end.

Am I missing something?

@ahocevar
Copy link
Member Author

The image in main is correct, but a failing test in #14923 revealed the problem, which was unnoticed because the change was below the image tolerance threshold.

@ahocevar
Copy link
Member Author

@jahow Now that #14923 is merged, I rebased this pull request and you can see the change in the diff of the reference image.

@jahow
Copy link
Contributor

jahow commented Jul 22, 2023

@jahow Now that #14923 is merged, I rebased this pull request and you can see the change in the diff of the reference image.

Ok yes, I understand, the image on main wasn't what the actual result looked like! But now doesn't that mean that this PR will significantly change the behavior for the textAlign property on lines? I mean, if that regression was there for a while, maybe people inverted end and start values in their application?

@ahocevar
Copy link
Member Author

ahocevar commented Jul 22, 2023

start and end is useful for applications that mix left-to-right and right-to-left text. I don't think there are many using that for aligning text along lines, otherwise someone would have created a bug report.

To avoid such regressions in the future, I lowered the test tolerance.

@ahocevar
Copy link
Member Author

I was wrong in assuming that this was a regression. The failed attempt to get left-to-right/right-to-left detection right was #11722, and that simply failed to fix the problem for text alignment along lines. So it was still broken after #11722, and is now fixed.

@ahocevar ahocevar changed the title Fix textAlign start/end conversion Fix textAlign start/end for placement: line Jul 22, 2023
@ahocevar
Copy link
Member Author

@jahow Were you suggesting to add a note about this in the release notes, or do you not agree with the fix in its entirety?

Copy link
Contributor

@jahow jahow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes a mention of this in the release notes wouldn't hurt I guess! Thanks for the fix and taking the time to make sense of all this :)

@ahocevar ahocevar merged commit 042cc9d into openlayers:main Aug 3, 2023
@ahocevar ahocevar deleted the text-align-start-end branch August 3, 2023 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

textAlign start/end regression
2 participants