Skip to content

Only shape point placement symbols - #8706

Merged
ryanhamley merged 4 commits into
masterfrom
text-max-width-newline
Sep 17, 2019
Merged

Only shape point placement symbols#8706
ryanhamley merged 4 commits into
masterfrom
text-max-width-newline

Conversation

@malwoodsantoro

@malwoodsantoro malwoodsantoro commented Aug 29, 2019

Copy link
Copy Markdown
Contributor

Launch Checklist

Fixes #8575

  • briefly describe the changes in this PR
    • This PR fixes a bug in which newline characters were being ignored if text-max-width was set to zero. This was caused by a faulty check in the shaping code which equated "no max width" with "no shaping should occur". This PR fixes the issue and corrects the check by explicitly checking for line and line-center symbol placement and not shaping those symbols (e.g. only shape text when the symbol is placed as a point).
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
    • this will need a port to native cc: @mapbox/gl-native

Changelog
Fix bug in which explicit newline characters were ignored when text-max-width was set to zero

@ryanhamley

ryanhamley commented Sep 3, 2019

Copy link
Copy Markdown
Contributor

This is working really well now. The only failing test is a render test text-max-width/unlimited which I believe to be an incorrect test. The test uses text-max-width: 0 and equates it with "unlimited" width, which is incorrect. My recommendation is to convert this to an explicit test of text-max-width: 0 with the expectation that there are line breaks on every possible whitespace character.

@ryanhamley
ryanhamley requested a review from mourner September 5, 2019 22:30
@ryanhamley
ryanhamley marked this pull request as ready for review September 5, 2019 22:30
@ryanhamley

Copy link
Copy Markdown
Contributor

#3725 is the PR that introduced the if (!maxWidth) check that is being changed here. I don't really understand why Chris went this particular direction. Making this change seems to be more consistent with the expected behavior and removes a confusing semantic equivalence between 0 max width and effectively infinite max width. I think this change is ok, but I just wanted to note the original PR in case.

@asheemmamoowala

Copy link
Copy Markdown
Contributor

The only failing test is a render test text-max-width/unlimited which I believe to be an incorrect test.
Making this change seems to be more consistent with the expected behavior and removes a confusing semantic equivalence between 0 max width and effectively infinite max width. I think this change is ok

@malwoodsantoro @ryanhamley It is unclear how this change affects existing styles that may be depending on this behavior. Maybe someone from @mapbox/map-design-team can comment on whether this change will require style changes for common use cases.

@ryanhamley

Copy link
Copy Markdown
Contributor

@mapbox/map-design-team Does anyone have feedback on this change?

@mourner mourner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me, pending benchmark results to make sure we don't regress on performance.

@samanpwbb samanpwbb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change makes sense and looks good 👍

@ryanhamley

Copy link
Copy Markdown
Contributor

Benchmarks look good

Screen Shot 2019-09-17 at 11 48 31 AM

Screen Shot 2019-09-17 at 11 48 40 AM

Screen Shot 2019-09-17 at 11 49 18 AM

Screen Shot 2019-09-17 at 11 49 26 AM

@ryanhamley

Copy link
Copy Markdown
Contributor

Visual Changes

This style with text-max-width: 0 and an explicit newline character in the text:
Screen Shot 2019-09-17 at 11 54 57 AM

Produces this output on master. Note that the newline is ignored.
Screen Shot 2019-09-17 at 11 54 23 AM

With this PR, this is the output. Since the text max width is 0, we break on every whitespace character.
Screen Shot 2019-09-17 at 11 53 34 AM

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.

newline character not respected if text-max-width is set to zero

5 participants