Skip to content

Conversation

@zmiao
Copy link
Contributor

@zmiao zmiao commented Oct 6, 2021

Launch Checklist

  • Fix: text-max-width is not correctly enforced #11088
    Previously, all the invisible codepoints' advances are not taken into account to text's accumulated width, even if the advance is not zero, such as white space ' '. The bug is revealed with Mono fonts as the white space from the font family has a bigger advance, 14 from two Mono fonts being tested, while the advance of default fonts is only 6.

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
Font Before After
League Mono Thin Screen Shot 2021-10-07 at 12 53 44 PM Screen Shot 2021-10-07 at 12 55 27 PM
Overpass Mono Regular Screen Shot 2021-10-07 at 12 52 18 PM Screen Shot 2021-10-07 at 12 55 52 PM
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fix line breaking behavior by taking the white space's advance into account.</changelog>

@mourner mourner linked an issue Oct 6, 2021 that may be closed by this pull request
zmiao added 6 commits October 6, 2021 23:14
…ble item, this is needed for badness calculations between current index to any previous potential breakables, so this tailed white space won't be accounted in the badness calculation.
@zmiao zmiao force-pushed the zmiao-fix-max-text-width branch from 1a3830b to 19b8d1c Compare October 7, 2021 09:37
@zmiao zmiao added the bug 🐞 label Oct 7, 2021
@zmiao zmiao requested review from alexshalamov and ansis October 7, 2021 10:08
@zmiao zmiao marked this pull request as ready for review October 7, 2021 10:08
@ansis
Copy link
Contributor

ansis commented Nov 17, 2021

This looks good to me! We should double check with some map designers that the change in behavior is acceptable. Styles designed with the old (wrong) behavior are going to look slightly different now. I think that is probably preferable to not fixing. An example with a non-mono font could be helpful for that.

@melanieimfeld
Copy link

Hi! @zmiao, trying to better understand the visual impact of this fix: Is it correct that the text-max-width will be reduced for any kind of symbol text? For example, text that is 10em wide (assuming the same text size and font) will appear narrower after this PR than before

@zmiao
Copy link
Contributor Author

zmiao commented Jan 7, 2022

@melanieimfeld that's right, the whole tendency of the line width per line will most probably become narrower than before if there are several empty spaces in the line. As with this change, the white space's width will be taken into account for line breaking determination.

@melanieimfeld
Copy link

@zmiao I was looking at streets-v11 to see if there are any impactful visual changes to the line break logic in long label names. I found relatively few changes:

before after
6-before 6-after
7-red-text-with-before 7-red-text-after
1-before 1-after

Our core styles are using DIN pro {} and Arial Unicode MS Regular as fallback, and for these fonts, the change in text width is a bit less extreme. Long text before and after example for DIN pro medium:

before after

That's to say that the change shouldn't impact our core styles in a noticeable way. The most noticeable change will be with styles using custom labels with a very long text. I can't think of any such styles in the wild, but I do know that @JonniWalker1977 has been experimenting with large text labels!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

text-max-width is not correctly enforced

3 participants