-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Rewrite measureText in writeTextToCanvas
#9765
Conversation
Thanks for the pull request @ebogo1!
Reviewers, don't forget to make sure that:
|
Noticing a bunch of |
There are definitely discrepancies between different browsers (noticeable with the "Offset label by distance" option in the Labels Sandcastle) - |
Can this be simplified further by using Here's another relevant StackOverflow issue: https://stackoverflow.com/questions/60347194/how-to-fit-text-to-a-precise-width-on-html-canvas. Assuming we still need a fallback for browsers that don't support Maybe it's my lack of Canvas2D experience showing, but the overall approach taken in |
Good call; seems to work fine with
It's written differently but the idea is the same; iterating one pixel at a time in the direction of that's being checked (i.e. up/down/left/right). It looks like the two options are a tradeoff: either we have a cleaner implementation but have inconsistent results in different browsers (and lose some browser support), or have the messier "brute force" implementation but get consistent browser results. If we use the brute force method as a fallback for unsupported browsers, there's also the question of whether the inconsistent results are acceptable; to me they look a bit bad in the above screenshots. I think if there's a fallback in the code at all we may as well use it all the time for consistency. |
var width4 = width * 4; | ||
var i, j; | ||
|
||
var ascent, descent; | ||
// Find the number of rows (from the top) until the first non-white pixel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to expand these because eslint doesn't like empty code blocks in loops..
looks like CI failed due to the #9769 timeouts.. |
// textBaseline needs to be set before the measureText call. It won't work otherwise. | ||
// It's magic. | ||
context2D.textBaseline = defaultValue(options.textBaseline, "bottom"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this no longer relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why it was - the measureText
function does not use the context's baseline anywhere. Maybe 9 years ago it worked differently? I didn't notice any changes when I removed this line.
Labels, Map Pins, and Clustering sandcastles look identical to main. The glyph spacing still leaves something to be desired, especially on Firefox, but that's out of scope for this PR. |
@lilleyse Should be good for final review. I pushed a small commit to the gulpfile now that there's no third party measureText file. |
Part of #9473.
This PR rewrites the code from Source/ThirdParty/measureText as a helper function in
writeTextToCanvas
. The point of the helper function is to extendCanvasRenderingContext2D.measureText()
withminx
andheight
properties (and truncate bounding box ascent/descent while we're there).The code is very inspired by the original third party function, but I believe stock ascent/descent values are good for what we're doing and I also think the loop to find
minx
was wrong in the third party version. We lose support forwriteTextToCanvas
on Firefox for Android though:@lilleyse Let me know what you think - if it's too close to the original I can rework it some more.
Tested the Labels Sandcastle on: