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

[samanpwbb]

mapbox-gl-js version: 1.1.0

browser: not browser specific

Steps to Trigger Behavior

1. Use a newline character \n in a text field value
2. Set text-max-width to zero

Link to Demonstration

https://jsbin.com/zawiyegehe/1/edit?html,output

Expected Behavior

Text wraps to second line after newline character:

Actual Behavior

Text does not wrap:

[mourner]

Good catch! Looks like we skip determining line breaks entirely if max-width is not set (regressed in #3725):

mapbox-gl-js/src/symbol/shaping.js

Lines 353 to 354 in 4ffd0e2

	if (!maxWidth)
	return [];

[ryanhamley]

@malwoodsantoro and I have been working on this bug together and I wanted to capture what we've discovered so far.

The proximate culprit is the check noted above

if (!maxWidth)
    return [];

Removing this check however runs into a related issue caused by this bit of code.

mapbox-gl-js/src/symbol/symbol_layout.js

Lines 186 to 188 in 75b9a40

	sizes.textMaxSize = unevaluatedLayoutValues['text-size'].possiblyEvaluate(new EvaluationParameters(18));
	const lineHeight = layout.get('text-line-height') * ONE_EM;

If symbol-placement is set to line or line-center, text-max-width is set to zero. This makes sense since text that follows a line shouldn't have breaks, but removing the if (!maxWidth) check causes the line to break on every whitespace character - exactly the opposite of what we want. This is confirmed by numerous failing line rendering tests if the if statement is removed.

A perhaps more correct way to write the if statement would be something like this:

// disable line breaking entirely on text-along-a-line
// point placed symbols will always be shaped
if (symbolPlacement !== 'point') {
    return [];
}

The only problem with this is that shaping.js does not have access to the layout instance or the symbol-placement property so we'll have to pass that in. Mal and I will work on submitting a PR in our meeting next week; I confirmed with Studio that this timeline is acceptable as this is not a high priority issue for them.

cc @asheemmamoowala