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

Fixed incorrect style props passed to Android Image when using children #8410

Conversation

jeanregisser
Copy link
Contributor

Hi there,

Here is a fix for #7538 (and #5085).

I had originally discovered this issue when using resizeMode through the style props. Although this might arguably be an incorrect usage (see #4759 (comment)) the same issue would happen with the tintColor and overlayColor style props.

To test this, you can render the following:

const imageContainerStyle = {width: 100, height: 100, backgroundColor: 'green', marginLeft: 10, marginTop: 10, };
const imageStyle = {flex: 1, width: undefined, height: undefined, resizeMode: 'contain', };

return (
  <View style={styles.container}>
    <View style={imageContainerStyle}>
      <Image style={imageStyle} source={
        {uri:'http://resizing.flixster.com/DeLpPTAwX3O2LszOpeaMHjbzuAw=/53x77/dkpu1ddg7pbsk.cloudfront.net/movie/11/16/47/11164719_ori.jpg'} 
      }>
      </Image>
    </View>
    <View style={imageContainerStyle}>
      <Image style={imageStyle} source={
        {uri:'http://resizing.flixster.com/DeLpPTAwX3O2LszOpeaMHjbzuAw=/53x77/dkpu1ddg7pbsk.cloudfront.net/movie/11/16/47/11164719_ori.jpg'}  
      }>
        <View style={{width: 20, height: 20, backgroundColor: 'blue'}} />
      </Image>
    </View>
  </View>
);

And verify that the second image (with the blue square) resizeMode is still correct and displays exactly as in the first image.
You can also substitute resizeMode with tintColor or overlayColor.

I tried to make the fix robust so it wouldn't introduce new issues if new style props were added to the Image component.
When the Image module is loaded, it intersects the View style props with the Image style props to extract the style props that are specific to the Image style. Right now the props are: resizeMode, tintColor and overlayColor (this one is Android only).

Let me know what you think.

@ghost
Copy link

ghost commented Jun 24, 2016

By analyzing the blame information on this pull request, we identified @andreicoman11 and @cornedor to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jun 24, 2016
@jeanregisser jeanregisser force-pushed the fix-android-image-incorrect-style-with-children branch from 3f11bb6 to e935258 Compare June 27, 2016 09:34
@andreicoman11
Copy link
Contributor

This looks good to me. Thanks!

@andreicoman11
Copy link
Contributor

@facebook-github-bot shipit

@facebook-github-bot facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jun 27, 2016
@ghost
Copy link

ghost commented Jun 27, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in 7f9ec4e Jun 27, 2016
bubblesunyum pushed a commit to iodine/react-native that referenced this pull request Aug 23, 2016
Summary:
Hi there,

Here is a fix for facebook#7538 (and facebook#5085).

I had originally discovered this issue when using `resizeMode` through the style props. Although this might arguably be an incorrect usage (see facebook#4759 (comment)) the same issue would happen with the `tintColor` and `overlayColor` style props.

To  test this, you can render the following:

```jsx
const imageContainerStyle = {width: 100, height: 100, backgroundColor: 'green', marginLeft: 10, marginTop: 10, };
const imageStyle = {flex: 1, width: undefined, height: undefined, resizeMode: 'contain', };

return (
  <View style={styles.container}>
    <View style={imageContainerStyle}>
      <Image style={imageStyle} source={
        {uri:'http://resizing.flixster.com/DeLpPTAwX3O2LszOpeaMHjbzuAw=/53x77/dkpu1ddg7pbsk.cloudfront.net/movie/11/16/47/11164719_ori.jpg'}
      }>
      </Image>
    </View>
    <View style={imageContainerStyle}>
      <Image style={imageStyle} source={
        {
Closes facebook#8410

Differential Revision: D3488010

Pulled By: andreicoman11

fbshipit-source-id: e9d1283cce8426c8878f9c3c66a43a2141232277
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
Summary:
Hi there,

Here is a fix for facebook#7538 (and facebook#5085).

I had originally discovered this issue when using `resizeMode` through the style props. Although this might arguably be an incorrect usage (see facebook#4759 (comment)) the same issue would happen with the `tintColor` and `overlayColor` style props.

To  test this, you can render the following:

```jsx
const imageContainerStyle = {width: 100, height: 100, backgroundColor: 'green', marginLeft: 10, marginTop: 10, };
const imageStyle = {flex: 1, width: undefined, height: undefined, resizeMode: 'contain', };

return (
  <View style={styles.container}>
    <View style={imageContainerStyle}>
      <Image style={imageStyle} source={
        {uri:'http://resizing.flixster.com/DeLpPTAwX3O2LszOpeaMHjbzuAw=/53x77/dkpu1ddg7pbsk.cloudfront.net/movie/11/16/47/11164719_ori.jpg'}
      }>
      </Image>
    </View>
    <View style={imageContainerStyle}>
      <Image style={imageStyle} source={
        {
Closes facebook#8410

Differential Revision: D3488010

Pulled By: andreicoman11

fbshipit-source-id: e9d1283cce8426c8878f9c3c66a43a2141232277
This pull request was closed.
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. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants