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

Fix: text shadow displays on iOS when textShadowOffset is {0,0} #24398

Closed
wants to merge 1 commit into from
Closed

Fix: text shadow displays on iOS when textShadowOffset is {0,0} #24398

wants to merge 1 commit into from

Conversation

woodpav
Copy link

@woodpav woodpav commented Apr 10, 2019

Summary

There is a problem rendering text shadows on iOS. If the offset of the text shadow is {width:0,height:0}, the shadow does not display. This prevents you from representing a light directly above the text. This occurs because a text shadow only renders if the offset is a non-zero CGRect {width:0,height:0}.

My change checks textShadowRadius instead. If textShadowRadius is not nan then the user is rendering a text shadow. There are no situations to render a shadow without textShadowRadius making it a good variable to check.

This PR fixes this stale issue: #17277

Changelog

[iOS] [Fixed] - Text shadow now displays when the textShadowOffset is {width:0,height:0}

Test Plan

textShadowOffset: {width:0,height: 0},
textShadowRadius: 10,
textShadowColor: "black"

Before, without fix:
Screen Shot 2019-04-10 at 2 12 16 PM

After with fix:
Screen Shot 2019-04-10 at 2 04 18 PM

@woodpav woodpav requested a review from shergin as a code owner April 10, 2019 18:50
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 10, 2019
@cpojer
Copy link
Contributor

cpojer commented Apr 10, 2019

Can you show a before and after screenshot of this behavior change and include it in the test plan section?

@woodpav
Copy link
Author

woodpav commented Apr 10, 2019

Done.

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Awesome, thank you :)

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @a-googler in 9b63b50.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Apr 11, 2019
@willnorris
Copy link

in case anyone's interested... this got attributed to @a-googler because of the use of "[email protected]" as author email. @woodpav, I'm just curious, do you intentionally have your git client configured that way?

@woodpav
Copy link
Author

woodpav commented Apr 11, 2019

I set it up without an email because I didn't have one I wanted made public. I guess it defaulted to that for some reason. I guess it's too late to change?

@cpojer
Copy link
Contributor

cpojer commented Apr 11, 2019 via email

@willnorris
Copy link

given that this has already been merged, yes it's too late. If you just want to keep your email private, see https://help.github.com/en/articles/about-commit-email-addresses for a special @users.noreply.github.com email you can use.

@woodpav
Copy link
Author

woodpav commented Apr 11, 2019

No worries @cpojer I cede the glory for isnan(_textShadowRadius)) to you!

@willnorris I have decided to step out of the shadows and add a developer email to my future commits. I will not fade from the annals of history anymore!

P.S. Thank you for your work on rn. Words alone cannot express my gratitude.

dsyang pushed a commit to dsyang/react-native that referenced this pull request Apr 12, 2019
…book#24398)

Summary:
There is a problem rendering text shadows on iOS. If the offset of the text shadow is `{width:0,height:0}`, the shadow does not display. This prevents you from representing a light directly above the text. This occurs because a text shadow only renders if the offset is a non-zero CGRect `{width:0,height:0}`.

My change checks `textShadowRadius` instead. If `textShadowRadius` is not nan then the user is rendering a text shadow. There are no situations to render a shadow without `textShadowRadius` making it a good variable to check.

This PR fixes this stale issue: facebook#17277

[iOS] [Fixed] - Text shadow now displays when the textShadowOffset is {width:0,height:0}
Pull Request resolved: facebook#24398

Differential Revision: D14890768

Pulled By: cpojer

fbshipit-source-id: a43b96a4a04a5603eede466abacd95c010d053e5
grabbou pushed a commit that referenced this pull request May 6, 2019
Summary:
There is a problem rendering text shadows on iOS. If the offset of the text shadow is `{width:0,height:0}`, the shadow does not display. This prevents you from representing a light directly above the text. This occurs because a text shadow only renders if the offset is a non-zero CGRect `{width:0,height:0}`.

My change checks `textShadowRadius` instead. If `textShadowRadius` is not nan then the user is rendering a text shadow. There are no situations to render a shadow without `textShadowRadius` making it a good variable to check.

This PR fixes this stale issue: #17277

[iOS] [Fixed] - Text shadow now displays when the textShadowOffset is {width:0,height:0}
Pull Request resolved: #24398

Differential Revision: D14890768

Pulled By: cpojer

fbshipit-source-id: a43b96a4a04a5603eede466abacd95c010d053e5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants