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

[0.54] <Text> rendering last line truncated (sometimes) #18258

Closed
fungilation opened this issue Mar 7, 2018 · 42 comments
Closed

[0.54] <Text> rendering last line truncated (sometimes) #18258

fungilation opened this issue Mar 7, 2018 · 42 comments
Labels
Impact: Regression Describes a behavior that used to work on a prior release, but stopped working recently. Platform: iOS iOS applications. Resolution: Locked This issue was locked by the bot.

Comments

@fungilation
Copy link

fungilation commented Mar 7, 2018

Environment

Environment:
  OS: macOS Sierra 10.12.6
  Node: 9.7.1
  Yarn: 1.5.1
  npm: 5.6.0
  Watchman: 4.9.0
  Xcode: Xcode 9.2 Build version 9C40b
  Android Studio: Not Found

Packages: (wanted => installed)
  react: ^16.3.0-alpha.1 => 16.3.0-alpha.1
  react-native: 0.54.0 => 0.54.0

Expected Behavior

<Text> views render without truncation where it shouldn't happen.

Actual Behavior

See these screenshots of my app. Pay attention to the 1st and 3rd Text views.

With numberOfLines={50} ellipsizeMode="middle" in <Text>:
screenshot 2018-03-07 13 54 25
Without numberOfLines={50} ellipsizeMode="middle" in <Text>:
screenshot 2018-03-07 14 09 59

I included 2 screenshots, with or without ellipsizeMode to show how it renders differently, but truncated in each case (in the 1st and 3rd Text views). The enclosing View wrapping the Text view preserve correct height if the Text view wasn't truncated, leaving an empty line on the bottom. Truncation happens on the second last line, as if numberOfLines is set to rendered lines - 1.

Notice that the unintended truncation doesn't happen with every Text view, only some. And the truncation happens with various lengths on what should have been the last line, so I'm not sure what's the condition for triggering it.

This unwanted truncation started on RN 0.54, not 0.53 and below. Obviously something about rendering calculation with the Text view rewrite/revamp in 0.54.

Steps to Reproduce

The views are rendered with a structure like

<Flatlist renderItem={({ item }) => !item || (
  <View style={{ margin: 10, padding: 8 }} key={'summary' + item.rowID}>
    <Text numberOfLines={50} ellipsizeMode="middle">.....</Text>
  </View>
)} />

For the 2 truncated Text views, these are the text contained within on array index 9 and 11:

Array(23)
0: "President Donald Trump’s new tariff orders on steel and aluminum perfectly illustrate Trump’s feral genius for playing to popular resentment of foreigners ― and the high costs of his"
1: "And the knee-jerk reaction to Trump’s orders shows how orthodox economists and the mainstream press refuse to grasp how trade really works, or what’s at stake."
2: "But if you want to appreciate true protectionism, take a good look at China’s entire economic system."
3: "What’s the nature of this trade war?"
4: "By comparison, the U.S., once the world’s leader, produces just over 70 million metric tons."
5: "In response to past complaints that resulted in tariffs against particular categories of dumped Chinese steel, China simply increased its exports of steel to other nations, such as South Korea, for re-export."
6: "But Trump did not just pull these tariffs out his ear."
7: "The second was a targeted tariff increase complemented by targeted quotas aimed at nations that were the source of the problem, most notably China."
8: "This was hardly a surprise."
9: "Trump ended the meeting by declaring to White House Chief of Staff John Kelly: “I know there are some people in the room right now that are upset."
10: "Trump adviser Peter Navarro was right on Fox News on Sunday when he referred to China as the deeper source of these gluts."
11: "Intermittently, the West has imposed selective anti-dumping duties against the Chinese, but has not challenged the predatory logic of the entire Chinese mercantilist system."
12: "(Like climate change, that long run has arrived abruptly.) Apple loves the fact that it can make its products in China, using a well-trained, cheap and docile work force one cut above slave labor."
13: "Usually, the influence of these players is sufficient to keep American presidents from taking too hard a line against Beijing."
14: "Under Trump, however, the internal White House politics changed."
15: "But the sheer simple-mindedness of Trump’s move to impose flat tariffs on all steel exporters stunned even the trade hawks."
16: "If the main problem is China, why retaliate against Canada, which actually buys more steel from the U.S. than it sells?"
17: "But Trump, in a stroke, managed to get Canada, the EU and China all on the same side."
18: "He goes for the most simplistic, vivid and demagogic remedy."
19: "But in the quest for a drastically different trade policy, Trump is about the last leader who would change course competently or constructively."
20: "A serious trade policy would go after the root of China’s state capitalism, enlisting every possible ally rather than alienating them."
21: "Raising tariffs on state-subsidized steel is a good and necessary part of the right policy."
22: "Robert Kuttner is co-editor of The American Prospect and professor at Brandeis University’s Heller School."

Screenshots above are on the iPhone X simulator. On physical device, it has different reproducibility but also occurs on different text. Screen width and number of words rendered per line is slightly different between simulator and on an actual iPhone X.

And for reference, my app package.json:

  "dependencies": {
    "babel-plugin-idx": "^2",
    "he": "^1.1.0",
    "lodash": "^4.17.2",
    "moment": "^2.19.0",
    "moment-timezone": "^0.5.10",
    "node-summary": "../node-summary",
    "react": "^16.3.0-alpha.1",
    "react-native": "0.54.0",
    "react-native-blur": "^3.2.0",
    "react-native-code-push": "^5.2.1",
    "react-native-easy-toast": "^1.0.9",
    "react-native-firebase": "^3.0",
    "react-native-fit-image": "^1.4.8",
    "react-native-highlight-words": "^1.0.1",
    "react-native-keep-awake": "^3.0.1",
    "react-native-linear-gradient": "^2.0.0",
    "react-native-modalbox": "^1.3.8",
    "react-native-orientation": "^3.1.3",
    "react-native-parallax-scroll-view": "../react-native-parallax-scroll-view",
    "react-native-safari-view": "^2.0.0",
    "react-native-sentry": "^0.34",
    "react-native-sha256": "^1.1.1",
    "react-native-status-bar-size": "^0.3.2",
    "react-native-swiper": "^1.5.10",
    "react-native-tooltip": "^5.2.0",
    "react-native-tts": "../react-native-tts",
    "react-native-vector-icons": "^4.1.1",
    "react-native-webview-bridge": "../react-native-webview-bridge-RN0.51",
    "react-redux": "^5.0.1",
    "redux": "^3.6.0",
    "redux-thunk": "^2.1.0"
  },
  "devDependencies": {
    "redux-logger": "^3.0.6"
  }
@fungilation
Copy link
Author

Here's a twist. I see that on both iPhone 6s (device) and iPhone 6 (simulator), no truncation on last line of <Text> can be reproduced on RN 0.54. Seems bizarre that the new Text component in 0.54 is affected on only iPhone X?

Could it be something to do with 3x pixels per point on the X? I haven't tested on plus versions of iPhones yet. Something to do with calculated number of lines expected by RN mismatching what's actually rendered?

screenshot 2018-03-07 21 12 26

@tikkichan4
Copy link

tikkichan4 commented Mar 8, 2018

+1, i am now not only facing this problem, but also the text component just cropped some of content at the last line of the text. It is not happened in other ios devices, but it happens in iPhoneX.
It is not accepted for my use case, so i am decided to downgrade to 0.53.0 until the problem being solved.

@fungilation
Copy link
Author

fungilation commented Mar 8, 2018

Ya I'm considering to downgrade but it's affecting only a fraction of Text comps on iPhone X for my app. It is still a glaring bug.

@shergin and @hovox since you are on the release notes for reimplementing Text in 0.54, can you investigate this please?

@hovox
Copy link
Contributor

hovox commented Mar 8, 2018

I have just fixed only cocoapods related issue.

@react-native-bot react-native-bot added the Platform: iOS iOS applications. label Mar 8, 2018
@fungilation
Copy link
Author

@shergin @grabbou a heads up if someone is looking into this please? This is a glaring bug affecting something as basic as the Text component.

@fungilation
Copy link
Author

Ok, I downgraded RN back to 0.53 since there's no uptake on fixing Text. Hopefully someone can look into this issue. 🙏🏼

@Rnorback
Copy link

We see the same behavior where the text doesn't wrap and get's chopped on an iPhone X. Here is our workaround for now.

message: {
        // FIXME: This is necessary to get things working on the iPhone X should be a patch for this soon
        paddingVertical: Constants.IS_IPHONE_X ? 9 : 0,
    },

@fungilation
Copy link
Author

fungilation commented Mar 14, 2018

@Rnorback I've tried playing with paddings already and it doesn't work. What gets chopped on the last line happens regardless of paddingVertical. paddingHorizontal would change where word wrap occurs so same Text may not get chopped, but chopping would happen on some other Text of different length.

I'd love to use a workaround if there's one. For now, I'm staying on RN 0.53.3.

@shergin
Copy link
Contributor

shergin commented Mar 14, 2018

a534672 supposed to fix that. Can anyone double check?

@fungilation
Copy link
Author

I confirm this is fixed (for my app and test cases) in RN 0.54.3. Thanks

@hramos hramos removed the Platform: macOS Building on macOS. label Mar 29, 2018
@chrmcg
Copy link

chrmcg commented Apr 11, 2018

I encountered a similar issue (with iPhone X only) on RN 0.55.2.
Downgrading to 0.54.3 fixed it.

@fungilation
Copy link
Author

That's interesting. Another regression??

I haven't upgraded to 0.55 yet, I'm still on 0.54.4 and no issue with iPhone X there.

I'm reopening. @chrmcg if you can provide more details like screenshot of issue on RN 0.55 that'd be useful.

@fungilation fungilation reopened this Apr 11, 2018
@chrmcg
Copy link

chrmcg commented Apr 12, 2018

Thanks @fungilation. Sorry for delayed response.

Expected behavior (screenshot from RN 0.54.3):
rn-0 54 3

Observed problem behavior (screenshot from RN 0.55.2):
rn-0 55 2

The difference is in the name of the first user -- note how on 55.2 it gets clipped instead of wrapped, although the vertical offset from the center is there as if it were going to be wrapped.

Relevant code for this snippet:

render() {
    return (
        <TouchableOpacity
            onPress={this.onSelect}
        >
            <View style={styles.container}>
                <View style={styles.trainerImageContainer}>
                    <PureCachedImage
                        style={styles.trainerImage}
                        source={imageOrPlaceholder(this.props.trainerAvatarUrl)} />
                </View>
                <View style={styles.trainerImageMargin} />
                <View style={styles.trainerNameContainer}>
                    <Text style={styles.trainerName}>
                        {this.props.trainerNameUpperCase}
                    </Text>
                </View>
            </View>
        </TouchableOpacity>
    );
}
const styles = StyleSheet.create({
    container: {
        alignItems: 'center',
        width: 116,
        height: 120,
        marginBottom: 35,
    },
    trainerImageContainer: {
        flex: 0,
    },
    trainerImageMargin: {
        height: 10,
        flex: 0,
    },
    trainerImage: {
        width: 80,
        height: 80,
        borderRadius: 80 / 2,
    },
    trainerNameContainer: {
        flex: 1,
        justifyContent: 'center',
    },
    trainerName: {
        fontFamily: Fonts.LIGHT,
        fontSize: 12,
        textAlign: 'center',
    },
});

Anecdotally this behavior appears to be rare but deterministic. If you rename one of these users, sometimes it fixes the problem; other times a different user starts exhibiting the problem. Pretty weird! Let me know if I can provide any more info.

@fungilation
Copy link
Author

Looks like the same issue I had. @shergin help please?

@musicode
Copy link

0.55.2 has the same issue

@fungilation
Copy link
Author

fungilation commented Apr 20, 2018

@grabbou or anyone else, this issue is dangling and a pretty important one to us who target the iPhone X (and Plus models too I'm guessing, which shares a device pixel of 3). Can correct tags/labels be added to this issue for more attention please?

@grabbou
Copy link
Contributor

grabbou commented May 14, 2018

That looks like regression to me. I am wondering what could've caused it.

CC: @shergin for his work on Text component and @emilsjolander for his Yoga layout work. Any of you recognizes what could be causing troubles here? (see the screenshot above)

@react-native-bot

This comment has been minimized.

@fungilation
Copy link
Author

No bot we just mentioned 0.55 reproduction.

@fungilation
Copy link
Author

As much as I don't want to... bump for how much we care about this.

@fungilation
Copy link
Author

Bump for some love? As I don't see this issue with Text getting any in https://github.com/react-native-community/react-native-releases/blob/master/CHANGELOG.md#056

@mpatric
Copy link
Contributor

mpatric commented Jun 14, 2018

Bump - this is still an issue on 0.55.4 without an apparent fix in 0.56?

@eliperkins
Copy link
Contributor

Yeah, this issue is blocking my company from upgrading beyond 0.53, which coincides with the <Text /> rewrite. I've done my best to understand what was done with the text rewrite, but I don't have have a thorough understanding of the new code, nor the reasoning as to why the text component was completely rewritten.

It's incredibly difficult to understand why and how a component has changed when it's simply a commit that lands in a project with little explanation. It's much easier to understand when there is ample conversation, and historical reasoning (usually in the form of an issue and PR), rather than a diff landing out of nowhere.

It would be great to have some sort of a post-mortem about the text rewrite, or further documentation on the internals of the text component so that we can help contribute back to this project once again.

@eliperkins
Copy link
Contributor

Additionally, the Old Version label should be removed from this issue as it affects the current version as well.

@charpeni charpeni added Impact: Regression Describes a behavior that used to work on a prior release, but stopped working recently. and removed ⏪Old Version labels Jun 14, 2018
@fungilation
Copy link
Author

Note that this issue appeared (for me) in RN 0.54.0, and was fixed by 0.54.3 or 0.54.4. My app WonderSwipe is on 0.54.4 without this issue. Haven't upgraded beyond as reported above, issue has reappeared in 0.55 and is a blocker

@hramos
Copy link
Contributor

hramos commented Jun 14, 2018

Has someone actually verified this on 0.56.0-rc.0? The CHANGELOG is not exhaustive, and we may have missed a commit related to this when writing it up.

@eliperkins
Copy link
Contributor

Updating to 0.56.0-rc.1 seems to address these issues for my application.

I don't know how to create a repro case for this to ensure that we don't run into this regression again, due to not knowing what commit fixed this issue...

@fungilation
Copy link
Author

fungilation commented Jun 21, 2018

@eliperkins Great to hear. For at least some optics for what's happening, can you post screenshots of before (0.55 or 0.53-) and after (0.56) with how Text rendering failed and worked correctly again?

@Dhanraj-bidchat

This comment has been minimized.

@hramos hramos added Resolution: Fixed A PR that fixes this issue has been merged. and removed Resolution: Fixed A PR that fixes this issue has been merged. labels Jun 28, 2018
@hramos
Copy link
Contributor

hramos commented Jun 28, 2018

@fungilation is it fixed in 0.56 in your case?

@fungilation
Copy link
Author

fungilation commented Jun 28, 2018

Haven't tested yet. Waiting for 0.56 release to drop

@hramos
Copy link
Contributor

hramos commented Jun 28, 2018

The 0.56 release candidate has been available for a couple of weeks now.

@fungilation
Copy link
Author

Release != RC. Does RCs work as a target for upgrade via react-native-git-upgrade?

@hramos
Copy link
Contributor

hramos commented Jun 28, 2018

You should be able to upgrade to the RC. We want as many people as possible, especially anyone filing issues in the repo, to be on the latest release candidate, to ensure we're not chasing issues that have already been fixed.

@fungilation
Copy link
Author

fungilation commented Jun 29, 2018

Ok, did a react-native-git-upgrade 0.56.0-rc.4. Looking good with my super methodical testing of scrolling through many, many text boxes in my app. Haven't seen a malformed Text on its last line, as I did previously (on 0.54.0) and I would have hit the bug at a rate of no less than 5-10%.

Closing this issue unless this somehow come back to haunt us past 0.56.0.

Would be nice to be able to trace back to what code changes fixed this, between last known bugged version of 0.55.2 / 0.55.3, to now fixed in 0.56.0-rc.1 / 0.56.0-rc.4. Mystery fixes are bad for future prevention.

@hramos
Copy link
Contributor

hramos commented Jun 29, 2018

I agree. We've been toying with the idea of requesting that a failing test case be added as part of filing some types of issues. That process would be a good way of demonstrating the failure in a minimal way, and help bubble up any commit that fixes it.

@fungilation
Copy link
Author

The trouble with this case is it's not known what exact condition causes it, other than certain text boxes on devices with devicepixel 3 (iPhone X) with certain height triggers it. In the apps we make/test. I've documented as much as I can on how an independent reproducible case could be made with data I see in my app, originally under "Steps to Reproduce".

@musicode
Copy link

0.57 has the same issue, test case like below:

<Text
  numberOfLines={3}
  style={{
    color: '#555',
    fontSize: 15,
    lineHeight: 22,
    textAlign: 'justify',
    marginTop: 10,
  }}
>
  没事看看这是妈妈的吗羡慕吗羡慕羡慕吗羡慕羡慕羡慕羡慕羡慕羡慕蜂蜜蜂蜜马芳吗多么吗羡慕羡慕羡慕羡慕羡慕们出门出门出门呢想念你承诺承诺发怒你想念羡慕羡慕羡慕吗羡慕羡慕羡慕羡慕多么多么美的你都目瞪口呆口袋空空的口袋宽松款的马德里队来了多么多么羡慕羡慕吗羡慕羡慕羡慕羡慕羡慕羡慕羡慕吗羡慕羡慕羡慕这么美...
</Text>

image

@grabbou @shergin @hramos

@fungilation
Copy link
Author

Nope, cannot reproduce in my app, with English. RN 0.57.0

@musicode
Copy link

I'm very sure this bug is still existed with 0.57, please test my code above on iPhone8 plus.

@hramos
Copy link
Contributor

hramos commented Sep 28, 2018

@musicode you may want to open a new issue to track the problem you're seeing. We rarely keep track of closed issues.

@facebook facebook locked as resolved and limited conversation to collaborators Jun 29, 2019
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jun 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Impact: Regression Describes a behavior that used to work on a prior release, but stopped working recently. Platform: iOS iOS applications. Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests