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

error running app with flatuiimplementation #12770

Closed
jcharbo opened this issue Mar 7, 2017 · 29 comments
Closed

error running app with flatuiimplementation #12770

jcharbo opened this issue Mar 7, 2017 · 29 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@jcharbo
Copy link

jcharbo commented Mar 7, 2017

I'm using RN 42 in genymotion. It works fine in IOS and android when not using the new experimental android nodes. When I add the code to android to "turn on" the new experimental nodes I get the following error.
thanks

Cannot add a child that doesn't have a YogaNode to a parent without a measure function! (Trying to add a 'ReactVirtualTextShadowNode' to a 'NativeViewWrapper')
addChildAt
    ReactShadowNode.java:172
addChildAt
    FlatShadowNode.java:219
addChildAt
    NativeViewWrapper.java:98
addChildAt
    FlatUIImplementation.java:505
setChildren
    FlatUIImplementation.java:185
setChildren
    UIManagerModule.java:312
invoke
    Method.java:-2
invoke
    BaseJavaModule.java:345
invoke
    JavaModuleWrapper.java:136
run
    NativeRunnable.java:-2
handleCallback
    Handler.java:739
dispatchMessage
    Handler.java:95
dispatchMessage
    MessageQueueThreadHandler.java:31
loop
    Looper.java:148
run
    MessageQueueThreadImpl.java:196
run
    Thread.java:818
@ericvicenti
Copy link
Contributor

ericvicenti commented Mar 8, 2017

I don't think you've provided enough details for core developers to reproduce and debug this issue. Can you please re-open this issue when you've identified exactly what your app is doing to cause this error, and provide the sample code that can cause it to happen?

@nihgwu
Copy link
Contributor

nihgwu commented Mar 8, 2017

@ericvicenti plz reopen this issue, Nodes works really well on RN below 0.42, it solves a lot of layout issues in may app, but I got this error since I upgrade RN to 0.42, and I have no idea what happened and how to resolve it, as it throws the error and stop the initialization of the app. So I've stopped upgrade the android version of my app. I thought it's still in experimental and will be resolved, but it's still there on RN0.43

@ericvicenti
Copy link
Contributor

cc @ahmedre

Can you provide any more details about how to get this error?

@gerbenis
Copy link

gerbenis commented Mar 8, 2017

@ericvicenti We've encountered the same problem on our project as well. It seems that this is a bug related to RN's Text component. To verify it we created a new project using react-native init, overriden ReactNativeHost's getUIImplementationProvider method to return new FlatUIImplementationProvider() in MainApplication.java and lauched the project. The app produced the following error:
screenshot 2017-03-08 03-02-56 popiet
The app lauches fine with Text components removed.

@jcharbo
Copy link
Author

jcharbo commented Mar 8, 2017

@ericvicenti I'd like to provide you more info but I have thousands of lines of code and this error does not give me enough info to find the underlying code issue. Hopefully the info from everyone above will be enough.

@ahmedre
Copy link

ahmedre commented Mar 8, 2017

cc @AaaChiuuu since i am traveling at the moment

@gerbenis
Copy link

@ericvicenti, @AaaChiuuu, any news on the issue? Moreover, I suggest to reopen it as there is a very clear way to reproduce it.

@ayc1
Copy link
Contributor

ayc1 commented Mar 17, 2017

I wasn't able to reproduce this issue on multiple applications after enabling nodes.

@gerbenis @jcharbo can you provide a clear minimum repro that includes js code?

@Ashoat
Copy link
Contributor

Ashoat commented Mar 20, 2017

I'm seeing the same exact issue using React-Native 42.3

@gavinyeah
Copy link

I'm also seeing this issue after upgrading to RN42.0

@gerbenis
Copy link

@ayc1, as I already mentioned, creating initial RN 42.0 project via CLI with nodes enabled in MainApplication.java was enough to reproduce the error. If it'll be of any help, I can create a repo and share it here.

@christianbach
Copy link

We to are seeing the same issue in our app. Worked prior to 0.42.0 and now we have the same yoga error. For us the error is related to the Text component.

@adogor
Copy link

adogor commented Mar 24, 2017

Hi @ericvicenti , could we reopen this issue ? Or do we have to create another one ?

Like @gerbenis with all versions after 0.42.0 even the react-native init doesn't work. So we have to stay at 0.41.x .

Thanks !

@dbof10
Copy link

dbof10 commented Mar 25, 2017

<View>
       <Image
         style={{height: 30, width: 30}}
         source={{uri : 'ic_home_black_24dp'}}/>
       <Text>Home</Text>
</View>

I did a sample to try FlatUIImplementation but hit the same issue

@ahmedre ahmedre reopened this Mar 25, 2017
@djohnkirby
Copy link

Experiencing the same thing after upgrading from 0.39 to 0.42

@raffij
Copy link

raffij commented Apr 6, 2017

Same experience with 0.43.1 with the base app created with react-native init

@chunghe
Copy link
Contributor

chunghe commented Apr 12, 2017

able to reproduce with 0.43.3.

following code renders correctly:

      <View style={{flex:1, backgroundColor: 'red'}}>
      </View>

following code displays red screen

      <View style={{flex:1, backgroundColor: 'red'}}>
        <Text>hello wolrd</Text>
      </View>

@nihgwu
Copy link
Contributor

nihgwu commented Apr 12, 2017

ping @ahmedre any progress on this? It's broken for quit a long time

@sunnylqm
Copy link
Contributor

ping @ericvicenti @AaaChiuuu

@aaronechiu
Copy link
Contributor

aaronechiu commented Apr 14, 2017

Hi, FlatUIImplementation is experimental and the React Native Core team is working on higher priority projects at this time. You'd want to stick with the regular UIImplementation. At this point, the earliest I'd expect us to get FlatUIImplementation working is some time next half.

@gmlion
Copy link

gmlion commented Apr 20, 2017

For what's worth, my experience is that FlatUIImplementation transformed an almost unusable Android app to one that is on par with the iOs version. Don't know anything about your priority, but this one for Android is a game changer, so I'm forced to stay with 0.41 until this bug is resolved

@raffij
Copy link

raffij commented Apr 20, 2017

@gmlion that's useful insight.

@sjkirby
Copy link
Contributor

sjkirby commented Apr 20, 2017

@gmlion I'm interested in what views comprise the Android surface of your app? Just to get an idea of where you are seeing performance improvements in the real world.

@gmlion
Copy link

gmlion commented Apr 20, 2017

The main culprit is most probably a stack of 10 nested images, but I can see performance degradation even on views on top of a navigation stack (I'm using react-native-router-flux).
For the first thing, on Android I used a GL surface and overlaid the images in the fragment shader before trying FlatUIImplementation.

@sjkirby
Copy link
Contributor

sjkirby commented Apr 20, 2017

I tracked down the issue and will push a fix later today.

@sjkirby
Copy link
Contributor

sjkirby commented Apr 28, 2017

Fixed in current source.

@jevakallio
Copy link
Contributor

I can confirm that c8bb422 seems to have fixed this in 0.46.0-rc2.

However, Nodes is still not working for me, now suffering from #14786 instead

@hramos
Copy link
Contributor

hramos commented Sep 21, 2017

Hi there! This issue is being closed because it has been inactive for a while. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. Either way, we're automatically closing issues after a period of inactivity. Please do not take it personally!

If you think this issue should definitely remain open, please let us know. The following information is helpful when it comes to determining if the issue should be re-opened:

  • Does the issue still reproduce on the latest release candidate? Post a comment with the version you tested.
  • If so, is there any information missing from the bug report? Post a comment with all the information required by the issue template.
  • Is there a pull request that addresses this issue? Post a comment with the PR number so we can follow up.

If you would like to work on a patch to fix the issue, contributions are very welcome! Read through the contribution guide, and feel free to hop into #react-native if you need help planning your contribution.

@hramos hramos added the Icebox label Sep 21, 2017
@hramos hramos closed this as completed Sep 21, 2017
@naveed89
Copy link

screenshot_1513325101

please help me same issue with me..

import React from 'react';
import {
Image,
Platform,
ScrollView,
StyleSheet,
Text,
TouchableOpacity,
View,
} from 'react-native';

export default class MenuScreen extends React.Component {
_onPressButton() {
Alert.alert('You tapped the button!')
}

_onLongPressButton() {
Alert.alert('You long-pressed the button!')
}

render() {
return (


<Image source={require('../assets/images/edit.png')} style={styles.Imageview}/>
/
Edit
/

    <TouchableOpacity onPress={this._onPressButton}>
    <Image source={require('../assets/images/newnote.png')} style={styles.Imageview}/>
    /*  <View style={styles.button}>

        <Text style={styles.buttonText}> New Note</Text>
      </View>*/
    </TouchableOpacity>

    <TouchableOpacity onPress={this._onPressButton}>
    <Image source={require('../assets/images/setting.png')} style={styles.Imageview}/>
      /* <View style={styles.button}>

        <Text style={styles.buttonText}>Setting</Text>
      </View>*/
    </TouchableOpacity>

    <TouchableOpacity onPress={this._onPressButton}>
    <Image source={require('../assets/images/delete.png')} style={styles.Imageview}/>
    /*  <View style={styles.button}>

        <Text style={styles.buttonText}>Delete</Text>
      </View> */
    </TouchableOpacity>

    <TouchableOpacity onPress={this._onPressButton}>
    /*  <View style={styles.button}>
    /*  <Image source={require('../assets/images/Done.jpg')} style={styles.Imageview}/>
        <Text style={styles.buttonText}>Save</Text>
      </View>*/
    </TouchableOpacity>

    <TouchableOpacity onPress={this._onPressButton}>
      <View style={styles.button}>
    /*  <Image source={require('../assets/images/sound.jpg')} style={styles.Imageview}/>
        <Text style={styles.buttonText}>Sound</Text>
      </View>*/
    </TouchableOpacity>

    <TouchableOpacity onPress={this._onPressButton}>
      <View style={styles.button}>
      /*<Image source={require('../assets/images/save to camera roll.jpg')} style={styles.Imageview}/>
        <Text style={styles.buttonText}>Camera</Text>
      </View>*/
    </TouchableOpacity>

    <TouchableOpacity onPress={this._onPressButton}>
    <Image source={require('../assets/images/calendar.png')} style={styles.Imageview}/>
    /*  <View style={styles.button}>
        <Text style={styles.buttonText}>Calender</Text>
      </View>*/
    </TouchableOpacity>

  </View>
);

}
}

const styles = StyleSheet.create({
container: {
paddingTop: 60,
alignItems: 'center'
},
button: {
marginBottom: 30,
width: 260,
alignItems: 'center',
backgroundColor: '#2196F3'
},
buttonText: {
padding: 20,
color: 'white'
},
Imageview: {
flex: 1,
// remove width and height to override fixed static size
justifyContent: 'center',
alignItems: 'center',

},
})

facebook-github-bot pushed a commit that referenced this issue Feb 27, 2018
Summary:
`letterSpacing` is completely missing from RN Android at the moment.

I've reviewed the `letterSpacing` implementations in #13199, #13877 and #16801 (that all seem to have stalled) and managed to put together an improved one based on #13199, updated to merge cleanly post 6114f86, that resolves the [issues](#13199 (comment)) I've identified with that code.

I believe this is the closest PR yet to a correct implementation of this feature, with a few caveats:

- As with the other PRs, this only works on Android >= 5.0 (silently falling back to no letter spacing on older versions). Is this acceptable for a RN feature, in general? Would a dev mode warning be desirable?
- The other PRs seem to have explored the space of potential solutions to the layout issue ([Android renders space _around_ glyphs](https://issuetracker.google.com/issues/37079859), iOS to the _right_ of each one) and come up empty, so I've opted to merely document the difference.
- I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers.
- The implementation in `ReactEditText` is only there to handle the placeholder text, as `ReactBaseTextShadowNode` already affects the input control's contents correctly.
  - I'm not sure whether `<TextInput>` is meant to respect `allowFontScaling`; I've taken my cue here from `ReactTextInputManager.setFontSize()`, and used the same units (SP) to interpret the value in `ReactEditText.setLetterSpacingPt()`.
  - I'm not sure whether `<TextInput>` is even meant to support `letterSpacing` - it doesn't actually work on iOS. I'm not going to be able to handle the Objective-C side of this, not as part of this PR at least.
- I have not added unit tests to `ReactTextTest` - is this desirable? I see that some other props such as `lineHeight` aren't covered there (unless I'm not looking in the right place).
- Overall, I'm new to this codebase, so it's likely I've missed something not mentioned here.

Note comment re: unit tests above; RNTester screenshots follow.

| iOS (existing functionality, amended test) | Android (new functionality & test) |
| - | - |
| <img src=https://user-images.githubusercontent.com/2246565/34458459-c8d59498-edcb-11e7-8c8f-e7426f723886.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458473-2a1ca368-edcc-11e7-9ce6-30c6d3a48660.png width=300> |

| iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) |
| - | - |
| <img src=https://user-images.githubusercontent.com/2246565/34458481-6c60a36e-edcc-11e7-9af5-9734dd722ced.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458486-8b3cdcf8-edcc-11e7-974b-25c6085fa674.png width=300> |

| iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) |
| - | - |
| <img src=https://user-images.githubusercontent.com/2246565/34458492-d69a77be-edcc-11e7-896f-21212621dbee.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458490-b3a1139e-edcc-11e7-88c8-79d4430d1514.png width=300> |

facebook/react-native-website#105 - this docs PR is edited slightly from what's in `TextStylePropTypes` here; happy to align either one to the other after a review.

[ANDROID] [FEATURE] [Text] - Implemented letterSpacing
Closes #17398

Reviewed By: mdvacca

Differential Revision: D6837718

Pulled By: hramos

fbshipit-source-id: 5c9d49e9cf4af6457b636416ce5fe15315aab72c
vincentriemer pushed a commit to vincentriemer/react-native-dom that referenced this issue Mar 6, 2018
Summary:
`letterSpacing` is completely missing from RN Android at the moment.

I've reviewed the `letterSpacing` implementations in #13199, #13877 and #16801 (that all seem to have stalled) and managed to put together an improved one based on #13199, updated to merge cleanly post facebook/react-native@6114f86, that resolves the [issues](facebook/react-native#13199 (comment)) I've identified with that code.

I believe this is the closest PR yet to a correct implementation of this feature, with a few caveats:

- As with the other PRs, this only works on Android >= 5.0 (silently falling back to no letter spacing on older versions). Is this acceptable for a RN feature, in general? Would a dev mode warning be desirable?
- The other PRs seem to have explored the space of potential solutions to the layout issue ([Android renders space _around_ glyphs](https://issuetracker.google.com/issues/37079859), iOS to the _right_ of each one) and come up empty, so I've opted to merely document the difference.
- I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](facebook/react-native#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers.
- The implementation in `ReactEditText` is only there to handle the placeholder text, as `ReactBaseTextShadowNode` already affects the input control's contents correctly.
  - I'm not sure whether `<TextInput>` is meant to respect `allowFontScaling`; I've taken my cue here from `ReactTextInputManager.setFontSize()`, and used the same units (SP) to interpret the value in `ReactEditText.setLetterSpacingPt()`.
  - I'm not sure whether `<TextInput>` is even meant to support `letterSpacing` - it doesn't actually work on iOS. I'm not going to be able to handle the Objective-C side of this, not as part of this PR at least.
- I have not added unit tests to `ReactTextTest` - is this desirable? I see that some other props such as `lineHeight` aren't covered there (unless I'm not looking in the right place).
- Overall, I'm new to this codebase, so it's likely I've missed something not mentioned here.

Note comment re: unit tests above; RNTester screenshots follow.

| iOS (existing functionality, amended test) | Android (new functionality & test) |
| - | - |
| <img src=https://user-images.githubusercontent.com/2246565/34458459-c8d59498-edcb-11e7-8c8f-e7426f723886.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458473-2a1ca368-edcc-11e7-9ce6-30c6d3a48660.png width=300> |

| iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) |
| - | - |
| <img src=https://user-images.githubusercontent.com/2246565/34458481-6c60a36e-edcc-11e7-9af5-9734dd722ced.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458486-8b3cdcf8-edcc-11e7-974b-25c6085fa674.png width=300> |

| iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) |
| - | - |
| <img src=https://user-images.githubusercontent.com/2246565/34458492-d69a77be-edcc-11e7-896f-21212621dbee.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458490-b3a1139e-edcc-11e7-88c8-79d4430d1514.png width=300> |

facebook/react-native-website#105 - this docs PR is edited slightly from what's in `TextStylePropTypes` here; happy to align either one to the other after a review.

[ANDROID] [FEATURE] [Text] - Implemented letterSpacing
Closes facebook/react-native#17398

Reviewed By: mdvacca

Differential Revision: D6837718

Pulled By: hramos

fbshipit-source-id: 5c9d49e9cf4af6457b636416ce5fe15315aab72c
facebook-github-bot pushed a commit that referenced this issue Mar 12, 2018
Summary:
`<Image resizeMode="repeat" />` for Android, matching the iOS implementation (#7968). (Non-goal: changing the component's API for finer-grained control / feature parity with CSS - this would be nice in the future)

As requested in e.g. #14158.

Given facebook/fresco#1575, and lacking the context to follow the specific recommendations in facebook/fresco#1575 (comment), I've opted for a minimal change within RN itself.

It's likely that performance can be improved by offloading this work to Fresco in some clever way; but I'm assuming that the present naive approach is still an improvement over a userland implementation with `onLayout` and multiple `<Image>` instances.

- Picking up on a TODO note in the existing code, I implemented `MultiPostprocessor` to allow arbitrary chaining of Fresco-compatible postprocessors inside `ReactImageView`.
- Rather than extensively refactor `ImageResizeMode`, `ReactImageManager` and `ReactImageView`, I mostly preserved the existing API that maps `resizeMode` values to [`ScaleType`](http://frescolib.org/javadoc/reference/com/facebook/drawee/drawable/ScalingUtils.ScaleType.html) instances, and simply added a second mapping, to [`TileMode`](https://developer.android.com/reference/android/graphics/Shader.TileMode.html).
- To match the iOS rendering exactly for oversized images, I found that scaling with a custom `ScaleType` was required - a kind of combination of `CENTER_INSIDE` and `FIT_START` which Fresco doesn't provide - so I implemented that as `ScaleTypeStartInside`. (This is, frankly, questionable as the default behaviour on iOS to begin with - but I am aiming for parity here)
- `resizeMode="repeat"` is therefore unpacked by the view manager to the effect of:
  ```js
     view.setScaleType(ScaleTypeStartInside.INSTANCE);
     view.setTileMode(Shader.TileMode.REPEAT);
   ```
  And the added postprocessing in the view (in case of a non-`CLAMP` tile mode) consists of waiting for layout, allocating a destination bitmap and painting the source bitmap with the requested tile mode and scale type.

Note that as in #17398 (comment), I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers.

Also, I'm happy to address any code style issues or other feedback; I'm new to this codebase and a very infrequent Android/Java coder.

Tested by enabling the relevant case in RNTester on Android.

| iOS | Android |
|-|-|
| <img src=https://user-images.githubusercontent.com/2246565/34461897-4e12008e-ee2f-11e7-8581-1dc0cc8f2779.png width=300>| <img src=https://user-images.githubusercontent.com/2246565/34461894-40b2c8ec-ee2f-11e7-8a8f-96704f3c8caa.png width=300> |

Docs update: facebook/react-native-website#106

[ANDROID] [FEATURE] [Image] - Implement resizeMode=repeat
Closes #17404

Reviewed By: achen1

Differential Revision: D7070329

Pulled By: mdvacca

fbshipit-source-id: 6a72fcbdcc7c7c2daf293dc1d8b6728f54ad0249
cpirich pushed a commit to tracemeinc/react-native that referenced this issue Mar 16, 2018
Summary:
`letterSpacing` is completely missing from RN Android at the moment.

I've reviewed the `letterSpacing` implementations in facebook#13199, facebook#13877 and facebook#16801 (that all seem to have stalled) and managed to put together an improved one based on facebook#13199, updated to merge cleanly post facebook@6114f86, that resolves the [issues](facebook#13199 (comment)) I've identified with that code.

I believe this is the closest PR yet to a correct implementation of this feature, with a few caveats:

- As with the other PRs, this only works on Android >= 5.0 (silently falling back to no letter spacing on older versions). Is this acceptable for a RN feature, in general? Would a dev mode warning be desirable?
- The other PRs seem to have explored the space of potential solutions to the layout issue ([Android renders space _around_ glyphs](https://issuetracker.google.com/issues/37079859), iOS to the _right_ of each one) and come up empty, so I've opted to merely document the difference.
- I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](facebook#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers.
- The implementation in `ReactEditText` is only there to handle the placeholder text, as `ReactBaseTextShadowNode` already affects the input control's contents correctly.
  - I'm not sure whether `<TextInput>` is meant to respect `allowFontScaling`; I've taken my cue here from `ReactTextInputManager.setFontSize()`, and used the same units (SP) to interpret the value in `ReactEditText.setLetterSpacingPt()`.
  - I'm not sure whether `<TextInput>` is even meant to support `letterSpacing` - it doesn't actually work on iOS. I'm not going to be able to handle the Objective-C side of this, not as part of this PR at least.
- I have not added unit tests to `ReactTextTest` - is this desirable? I see that some other props such as `lineHeight` aren't covered there (unless I'm not looking in the right place).
- Overall, I'm new to this codebase, so it's likely I've missed something not mentioned here.

Note comment re: unit tests above; RNTester screenshots follow.

| iOS (existing functionality, amended test) | Android (new functionality & test) |
| - | - |
| <img src=https://user-images.githubusercontent.com/2246565/34458459-c8d59498-edcb-11e7-8c8f-e7426f723886.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458473-2a1ca368-edcc-11e7-9ce6-30c6d3a48660.png width=300> |

| iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) |
| - | - |
| <img src=https://user-images.githubusercontent.com/2246565/34458481-6c60a36e-edcc-11e7-9af5-9734dd722ced.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458486-8b3cdcf8-edcc-11e7-974b-25c6085fa674.png width=300> |

| iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) |
| - | - |
| <img src=https://user-images.githubusercontent.com/2246565/34458492-d69a77be-edcc-11e7-896f-21212621dbee.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458490-b3a1139e-edcc-11e7-88c8-79d4430d1514.png width=300> |

facebook/react-native-website#105 - this docs PR is edited slightly from what's in `TextStylePropTypes` here; happy to align either one to the other after a review.

[ANDROID] [FEATURE] [Text] - Implemented letterSpacing
Closes facebook#17398

Reviewed By: mdvacca

Differential Revision: D6837718

Pulled By: hramos

fbshipit-source-id: 5c9d49e9cf4af6457b636416ce5fe15315aab72c
guyca pushed a commit to wix-playground/react-native that referenced this issue Mar 29, 2018
Summary:
`letterSpacing` is completely missing from RN Android at the moment.

I've reviewed the `letterSpacing` implementations in facebook#13199, facebook#13877 and facebook#16801 (that all seem to have stalled) and managed to put together an improved one based on facebook#13199, updated to merge cleanly post facebook@6114f86, that resolves the [issues](facebook#13199 (comment)) I've identified with that code.

I believe this is the closest PR yet to a correct implementation of this feature, with a few caveats:

- As with the other PRs, this only works on Android >= 5.0 (silently falling back to no letter spacing on older versions). Is this acceptable for a RN feature, in general? Would a dev mode warning be desirable?
- The other PRs seem to have explored the space of potential solutions to the layout issue ([Android renders space _around_ glyphs](https://issuetracker.google.com/issues/37079859), iOS to the _right_ of each one) and come up empty, so I've opted to merely document the difference.
- I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](facebook#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers.
- The implementation in `ReactEditText` is only there to handle the placeholder text, as `ReactBaseTextShadowNode` already affects the input control's contents correctly.
  - I'm not sure whether `<TextInput>` is meant to respect `allowFontScaling`; I've taken my cue here from `ReactTextInputManager.setFontSize()`, and used the same units (SP) to interpret the value in `ReactEditText.setLetterSpacingPt()`.
  - I'm not sure whether `<TextInput>` is even meant to support `letterSpacing` - it doesn't actually work on iOS. I'm not going to be able to handle the Objective-C side of this, not as part of this PR at least.
- I have not added unit tests to `ReactTextTest` - is this desirable? I see that some other props such as `lineHeight` aren't covered there (unless I'm not looking in the right place).
- Overall, I'm new to this codebase, so it's likely I've missed something not mentioned here.

Note comment re: unit tests above; RNTester screenshots follow.

| iOS (existing functionality, amended test) | Android (new functionality & test) |
| - | - |
| <img src=https://user-images.githubusercontent.com/2246565/34458459-c8d59498-edcb-11e7-8c8f-e7426f723886.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458473-2a1ca368-edcc-11e7-9ce6-30c6d3a48660.png width=300> |

| iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) |
| - | - |
| <img src=https://user-images.githubusercontent.com/2246565/34458481-6c60a36e-edcc-11e7-9af5-9734dd722ced.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458486-8b3cdcf8-edcc-11e7-974b-25c6085fa674.png width=300> |

| iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) |
| - | - |
| <img src=https://user-images.githubusercontent.com/2246565/34458492-d69a77be-edcc-11e7-896f-21212621dbee.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458490-b3a1139e-edcc-11e7-88c8-79d4430d1514.png width=300> |

facebook/react-native-website#105 - this docs PR is edited slightly from what's in `TextStylePropTypes` here; happy to align either one to the other after a review.

[ANDROID] [FEATURE] [Text] - Implemented letterSpacing
Closes facebook#17398

Reviewed By: mdvacca

Differential Revision: D6837718

Pulled By: hramos

fbshipit-source-id: 5c9d49e9cf4af6457b636416ce5fe15315aab72c
hamaron pushed a commit to hamaron/react-native that referenced this issue Apr 9, 2018
Summary:
`<Image resizeMode="repeat" />` for Android, matching the iOS implementation (facebook#7968). (Non-goal: changing the component's API for finer-grained control / feature parity with CSS - this would be nice in the future)

As requested in e.g. facebook#14158.

Given facebook/fresco#1575, and lacking the context to follow the specific recommendations in facebook/fresco#1575 (comment), I've opted for a minimal change within RN itself.

It's likely that performance can be improved by offloading this work to Fresco in some clever way; but I'm assuming that the present naive approach is still an improvement over a userland implementation with `onLayout` and multiple `<Image>` instances.

- Picking up on a TODO note in the existing code, I implemented `MultiPostprocessor` to allow arbitrary chaining of Fresco-compatible postprocessors inside `ReactImageView`.
- Rather than extensively refactor `ImageResizeMode`, `ReactImageManager` and `ReactImageView`, I mostly preserved the existing API that maps `resizeMode` values to [`ScaleType`](http://frescolib.org/javadoc/reference/com/facebook/drawee/drawable/ScalingUtils.ScaleType.html) instances, and simply added a second mapping, to [`TileMode`](https://developer.android.com/reference/android/graphics/Shader.TileMode.html).
- To match the iOS rendering exactly for oversized images, I found that scaling with a custom `ScaleType` was required - a kind of combination of `CENTER_INSIDE` and `FIT_START` which Fresco doesn't provide - so I implemented that as `ScaleTypeStartInside`. (This is, frankly, questionable as the default behaviour on iOS to begin with - but I am aiming for parity here)
- `resizeMode="repeat"` is therefore unpacked by the view manager to the effect of:
  ```js
     view.setScaleType(ScaleTypeStartInside.INSTANCE);
     view.setTileMode(Shader.TileMode.REPEAT);
   ```
  And the added postprocessing in the view (in case of a non-`CLAMP` tile mode) consists of waiting for layout, allocating a destination bitmap and painting the source bitmap with the requested tile mode and scale type.

Note that as in facebook#17398 (comment), I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](facebook#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers.

Also, I'm happy to address any code style issues or other feedback; I'm new to this codebase and a very infrequent Android/Java coder.

Tested by enabling the relevant case in RNTester on Android.

| iOS | Android |
|-|-|
| <img src=https://user-images.githubusercontent.com/2246565/34461897-4e12008e-ee2f-11e7-8581-1dc0cc8f2779.png width=300>| <img src=https://user-images.githubusercontent.com/2246565/34461894-40b2c8ec-ee2f-11e7-8a8f-96704f3c8caa.png width=300> |

Docs update: facebook/react-native-website#106

[ANDROID] [FEATURE] [Image] - Implement resizeMode=repeat
Closes facebook#17404

Reviewed By: achen1

Differential Revision: D7070329

Pulled By: mdvacca

fbshipit-source-id: 6a72fcbdcc7c7c2daf293dc1d8b6728f54ad0249
campsafari pushed a commit to exozet/react-native that referenced this issue Apr 11, 2018
Summary:
`<Image resizeMode="repeat" />` for Android, matching the iOS implementation (facebook#7968). (Non-goal: changing the component's API for finer-grained control / feature parity with CSS - this would be nice in the future)

As requested in e.g. facebook#14158.

Given facebook/fresco#1575, and lacking the context to follow the specific recommendations in facebook/fresco#1575 (comment), I've opted for a minimal change within RN itself.

It's likely that performance can be improved by offloading this work to Fresco in some clever way; but I'm assuming that the present naive approach is still an improvement over a userland implementation with `onLayout` and multiple `<Image>` instances.

- Picking up on a TODO note in the existing code, I implemented `MultiPostprocessor` to allow arbitrary chaining of Fresco-compatible postprocessors inside `ReactImageView`.
- Rather than extensively refactor `ImageResizeMode`, `ReactImageManager` and `ReactImageView`, I mostly preserved the existing API that maps `resizeMode` values to [`ScaleType`](http://frescolib.org/javadoc/reference/com/facebook/drawee/drawable/ScalingUtils.ScaleType.html) instances, and simply added a second mapping, to [`TileMode`](https://developer.android.com/reference/android/graphics/Shader.TileMode.html).
- To match the iOS rendering exactly for oversized images, I found that scaling with a custom `ScaleType` was required - a kind of combination of `CENTER_INSIDE` and `FIT_START` which Fresco doesn't provide - so I implemented that as `ScaleTypeStartInside`. (This is, frankly, questionable as the default behaviour on iOS to begin with - but I am aiming for parity here)
- `resizeMode="repeat"` is therefore unpacked by the view manager to the effect of:
  ```js
     view.setScaleType(ScaleTypeStartInside.INSTANCE);
     view.setTileMode(Shader.TileMode.REPEAT);
   ```
  And the added postprocessing in the view (in case of a non-`CLAMP` tile mode) consists of waiting for layout, allocating a destination bitmap and painting the source bitmap with the requested tile mode and scale type.

Note that as in facebook#17398 (comment), I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](facebook#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers.

Also, I'm happy to address any code style issues or other feedback; I'm new to this codebase and a very infrequent Android/Java coder.

Tested by enabling the relevant case in RNTester on Android.

| iOS | Android |
|-|-|
| <img src=https://user-images.githubusercontent.com/2246565/34461897-4e12008e-ee2f-11e7-8581-1dc0cc8f2779.png width=300>| <img src=https://user-images.githubusercontent.com/2246565/34461894-40b2c8ec-ee2f-11e7-8a8f-96704f3c8caa.png width=300> |

Docs update: facebook/react-native-website#106

[ANDROID] [FEATURE] [Image] - Implement resizeMode=repeat
Closes facebook#17404

Reviewed By: achen1

Differential Revision: D7070329

Pulled By: mdvacca

fbshipit-source-id: 6a72fcbdcc7c7c2daf293dc1d8b6728f54ad0249
LukeDurrant pushed a commit to LukeDurrant/react-native that referenced this issue Apr 11, 2018
Summary:
`<Image resizeMode="repeat" />` for Android, matching the iOS implementation (facebook#7968). (Non-goal: changing the component's API for finer-grained control / feature parity with CSS - this would be nice in the future)

As requested in e.g. facebook#14158.

Given facebook/fresco#1575, and lacking the context to follow the specific recommendations in facebook/fresco#1575 (comment), I've opted for a minimal change within RN itself.

It's likely that performance can be improved by offloading this work to Fresco in some clever way; but I'm assuming that the present naive approach is still an improvement over a userland implementation with `onLayout` and multiple `<Image>` instances.

- Picking up on a TODO note in the existing code, I implemented `MultiPostprocessor` to allow arbitrary chaining of Fresco-compatible postprocessors inside `ReactImageView`.
- Rather than extensively refactor `ImageResizeMode`, `ReactImageManager` and `ReactImageView`, I mostly preserved the existing API that maps `resizeMode` values to [`ScaleType`](http://frescolib.org/javadoc/reference/com/facebook/drawee/drawable/ScalingUtils.ScaleType.html) instances, and simply added a second mapping, to [`TileMode`](https://developer.android.com/reference/android/graphics/Shader.TileMode.html).
- To match the iOS rendering exactly for oversized images, I found that scaling with a custom `ScaleType` was required - a kind of combination of `CENTER_INSIDE` and `FIT_START` which Fresco doesn't provide - so I implemented that as `ScaleTypeStartInside`. (This is, frankly, questionable as the default behaviour on iOS to begin with - but I am aiming for parity here)
- `resizeMode="repeat"` is therefore unpacked by the view manager to the effect of:
  ```js
     view.setScaleType(ScaleTypeStartInside.INSTANCE);
     view.setTileMode(Shader.TileMode.REPEAT);
   ```
  And the added postprocessing in the view (in case of a non-`CLAMP` tile mode) consists of waiting for layout, allocating a destination bitmap and painting the source bitmap with the requested tile mode and scale type.

Note that as in facebook#17398 (comment), I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](facebook#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers.

Also, I'm happy to address any code style issues or other feedback; I'm new to this codebase and a very infrequent Android/Java coder.

Tested by enabling the relevant case in RNTester on Android.

| iOS | Android |
|-|-|
| <img src=https://user-images.githubusercontent.com/2246565/34461897-4e12008e-ee2f-11e7-8581-1dc0cc8f2779.png width=300>| <img src=https://user-images.githubusercontent.com/2246565/34461894-40b2c8ec-ee2f-11e7-8a8f-96704f3c8caa.png width=300> |

Docs update: facebook/react-native-website#106

[ANDROID] [FEATURE] [Image] - Implement resizeMode=repeat
Closes facebook#17404

Reviewed By: achen1

Differential Revision: D7070329

Pulled By: mdvacca

fbshipit-source-id: 6a72fcbdcc7c7c2daf293dc1d8b6728f54ad0249
LukeDurrant pushed a commit to LukeDurrant/react-native that referenced this issue Apr 11, 2018
Summary:
`<Image resizeMode="repeat" />` for Android, matching the iOS implementation (facebook#7968). (Non-goal: changing the component's API for finer-grained control / feature parity with CSS - this would be nice in the future)

As requested in e.g. facebook#14158.

Given facebook/fresco#1575, and lacking the context to follow the specific recommendations in facebook/fresco#1575 (comment), I've opted for a minimal change within RN itself.

It's likely that performance can be improved by offloading this work to Fresco in some clever way; but I'm assuming that the present naive approach is still an improvement over a userland implementation with `onLayout` and multiple `<Image>` instances.

- Picking up on a TODO note in the existing code, I implemented `MultiPostprocessor` to allow arbitrary chaining of Fresco-compatible postprocessors inside `ReactImageView`.
- Rather than extensively refactor `ImageResizeMode`, `ReactImageManager` and `ReactImageView`, I mostly preserved the existing API that maps `resizeMode` values to [`ScaleType`](http://frescolib.org/javadoc/reference/com/facebook/drawee/drawable/ScalingUtils.ScaleType.html) instances, and simply added a second mapping, to [`TileMode`](https://developer.android.com/reference/android/graphics/Shader.TileMode.html).
- To match the iOS rendering exactly for oversized images, I found that scaling with a custom `ScaleType` was required - a kind of combination of `CENTER_INSIDE` and `FIT_START` which Fresco doesn't provide - so I implemented that as `ScaleTypeStartInside`. (This is, frankly, questionable as the default behaviour on iOS to begin with - but I am aiming for parity here)
- `resizeMode="repeat"` is therefore unpacked by the view manager to the effect of:
  ```js
     view.setScaleType(ScaleTypeStartInside.INSTANCE);
     view.setTileMode(Shader.TileMode.REPEAT);
   ```
  And the added postprocessing in the view (in case of a non-`CLAMP` tile mode) consists of waiting for layout, allocating a destination bitmap and painting the source bitmap with the requested tile mode and scale type.

Note that as in facebook#17398 (comment), I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](facebook#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers.

Also, I'm happy to address any code style issues or other feedback; I'm new to this codebase and a very infrequent Android/Java coder.

Tested by enabling the relevant case in RNTester on Android.

| iOS | Android |
|-|-|
| <img src=https://user-images.githubusercontent.com/2246565/34461897-4e12008e-ee2f-11e7-8581-1dc0cc8f2779.png width=300>| <img src=https://user-images.githubusercontent.com/2246565/34461894-40b2c8ec-ee2f-11e7-8a8f-96704f3c8caa.png width=300> |

Docs update: facebook/react-native-website#106

[ANDROID] [FEATURE] [Image] - Implement resizeMode=repeat
Closes facebook#17404

Reviewed By: achen1

Differential Revision: D7070329

Pulled By: mdvacca

fbshipit-source-id: 6a72fcbdcc7c7c2daf293dc1d8b6728f54ad0249
bunnyc1986 pushed a commit to bunnyc1986/react-native that referenced this issue May 11, 2018
Summary:
`<Image resizeMode="repeat" />` for Android, matching the iOS implementation (facebook#7968). (Non-goal: changing the component's API for finer-grained control / feature parity with CSS - this would be nice in the future)

As requested in e.g. facebook#14158.

Given facebook/fresco#1575, and lacking the context to follow the specific recommendations in facebook/fresco#1575 (comment), I've opted for a minimal change within RN itself.

It's likely that performance can be improved by offloading this work to Fresco in some clever way; but I'm assuming that the present naive approach is still an improvement over a userland implementation with `onLayout` and multiple `<Image>` instances.

- Picking up on a TODO note in the existing code, I implemented `MultiPostprocessor` to allow arbitrary chaining of Fresco-compatible postprocessors inside `ReactImageView`.
- Rather than extensively refactor `ImageResizeMode`, `ReactImageManager` and `ReactImageView`, I mostly preserved the existing API that maps `resizeMode` values to [`ScaleType`](http://frescolib.org/javadoc/reference/com/facebook/drawee/drawable/ScalingUtils.ScaleType.html) instances, and simply added a second mapping, to [`TileMode`](https://developer.android.com/reference/android/graphics/Shader.TileMode.html).
- To match the iOS rendering exactly for oversized images, I found that scaling with a custom `ScaleType` was required - a kind of combination of `CENTER_INSIDE` and `FIT_START` which Fresco doesn't provide - so I implemented that as `ScaleTypeStartInside`. (This is, frankly, questionable as the default behaviour on iOS to begin with - but I am aiming for parity here)
- `resizeMode="repeat"` is therefore unpacked by the view manager to the effect of:
  ```js
     view.setScaleType(ScaleTypeStartInside.INSTANCE);
     view.setTileMode(Shader.TileMode.REPEAT);
   ```
  And the added postprocessing in the view (in case of a non-`CLAMP` tile mode) consists of waiting for layout, allocating a destination bitmap and painting the source bitmap with the requested tile mode and scale type.

Note that as in facebook#17398 (comment), I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](facebook#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers.

Also, I'm happy to address any code style issues or other feedback; I'm new to this codebase and a very infrequent Android/Java coder.

Tested by enabling the relevant case in RNTester on Android.

| iOS | Android |
|-|-|
| <img src=https://user-images.githubusercontent.com/2246565/34461897-4e12008e-ee2f-11e7-8581-1dc0cc8f2779.png width=300>| <img src=https://user-images.githubusercontent.com/2246565/34461894-40b2c8ec-ee2f-11e7-8a8f-96704f3c8caa.png width=300> |

Docs update: facebook/react-native-website#106

[ANDROID] [FEATURE] [Image] - Implement resizeMode=repeat
Closes facebook#17404

Reviewed By: achen1

Differential Revision: D7070329

Pulled By: mdvacca

fbshipit-source-id: 6a72fcbdcc7c7c2daf293dc1d8b6728f54ad0249
buoyad pushed a commit to keybase/react-native that referenced this issue Jun 5, 2018
Summary:
`<Image resizeMode="repeat" />` for Android, matching the iOS implementation (facebook#7968). (Non-goal: changing the component's API for finer-grained control / feature parity with CSS - this would be nice in the future)

As requested in e.g. facebook#14158.

Given facebook/fresco#1575, and lacking the context to follow the specific recommendations in facebook/fresco#1575 (comment), I've opted for a minimal change within RN itself.

It's likely that performance can be improved by offloading this work to Fresco in some clever way; but I'm assuming that the present naive approach is still an improvement over a userland implementation with `onLayout` and multiple `<Image>` instances.

- Picking up on a TODO note in the existing code, I implemented `MultiPostprocessor` to allow arbitrary chaining of Fresco-compatible postprocessors inside `ReactImageView`.
- Rather than extensively refactor `ImageResizeMode`, `ReactImageManager` and `ReactImageView`, I mostly preserved the existing API that maps `resizeMode` values to [`ScaleType`](http://frescolib.org/javadoc/reference/com/facebook/drawee/drawable/ScalingUtils.ScaleType.html) instances, and simply added a second mapping, to [`TileMode`](https://developer.android.com/reference/android/graphics/Shader.TileMode.html).
- To match the iOS rendering exactly for oversized images, I found that scaling with a custom `ScaleType` was required - a kind of combination of `CENTER_INSIDE` and `FIT_START` which Fresco doesn't provide - so I implemented that as `ScaleTypeStartInside`. (This is, frankly, questionable as the default behaviour on iOS to begin with - but I am aiming for parity here)
- `resizeMode="repeat"` is therefore unpacked by the view manager to the effect of:
  ```js
     view.setScaleType(ScaleTypeStartInside.INSTANCE);
     view.setTileMode(Shader.TileMode.REPEAT);
   ```
  And the added postprocessing in the view (in case of a non-`CLAMP` tile mode) consists of waiting for layout, allocating a destination bitmap and painting the source bitmap with the requested tile mode and scale type.

Note that as in facebook#17398 (comment), I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](facebook#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers.

Also, I'm happy to address any code style issues or other feedback; I'm new to this codebase and a very infrequent Android/Java coder.

Tested by enabling the relevant case in RNTester on Android.

| iOS | Android |
|-|-|
| <img src=https://user-images.githubusercontent.com/2246565/34461897-4e12008e-ee2f-11e7-8581-1dc0cc8f2779.png width=300>| <img src=https://user-images.githubusercontent.com/2246565/34461894-40b2c8ec-ee2f-11e7-8a8f-96704f3c8caa.png width=300> |

Docs update: facebook/react-native-website#106

[ANDROID] [FEATURE] [Image] - Implement resizeMode=repeat
Closes facebook#17404

Reviewed By: achen1

Differential Revision: D7070329

Pulled By: mdvacca

fbshipit-source-id: 6a72fcbdcc7c7c2daf293dc1d8b6728f54ad0249
(cherry picked from commit 0459e4f)
lnikkila pushed a commit to Boulevard/react-native that referenced this issue Jun 6, 2018
Summary:
`<Image resizeMode="repeat" />` for Android, matching the iOS implementation (facebook#7968). (Non-goal: changing the component's API for finer-grained control / feature parity with CSS - this would be nice in the future)

As requested in e.g. facebook#14158.

Given facebook/fresco#1575, and lacking the context to follow the specific recommendations in facebook/fresco#1575 (comment), I've opted for a minimal change within RN itself.

It's likely that performance can be improved by offloading this work to Fresco in some clever way; but I'm assuming that the present naive approach is still an improvement over a userland implementation with `onLayout` and multiple `<Image>` instances.

- Picking up on a TODO note in the existing code, I implemented `MultiPostprocessor` to allow arbitrary chaining of Fresco-compatible postprocessors inside `ReactImageView`.
- Rather than extensively refactor `ImageResizeMode`, `ReactImageManager` and `ReactImageView`, I mostly preserved the existing API that maps `resizeMode` values to [`ScaleType`](http://frescolib.org/javadoc/reference/com/facebook/drawee/drawable/ScalingUtils.ScaleType.html) instances, and simply added a second mapping, to [`TileMode`](https://developer.android.com/reference/android/graphics/Shader.TileMode.html).
- To match the iOS rendering exactly for oversized images, I found that scaling with a custom `ScaleType` was required - a kind of combination of `CENTER_INSIDE` and `FIT_START` which Fresco doesn't provide - so I implemented that as `ScaleTypeStartInside`. (This is, frankly, questionable as the default behaviour on iOS to begin with - but I am aiming for parity here)
- `resizeMode="repeat"` is therefore unpacked by the view manager to the effect of:
  ```js
     view.setScaleType(ScaleTypeStartInside.INSTANCE);
     view.setTileMode(Shader.TileMode.REPEAT);
   ```
  And the added postprocessing in the view (in case of a non-`CLAMP` tile mode) consists of waiting for layout, allocating a destination bitmap and painting the source bitmap with the requested tile mode and scale type.

Note that as in facebook#17398 (comment), I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](facebook#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers.

Also, I'm happy to address any code style issues or other feedback; I'm new to this codebase and a very infrequent Android/Java coder.

Tested by enabling the relevant case in RNTester on Android.

| iOS | Android |
|-|-|
| <img src=https://user-images.githubusercontent.com/2246565/34461897-4e12008e-ee2f-11e7-8581-1dc0cc8f2779.png width=300>| <img src=https://user-images.githubusercontent.com/2246565/34461894-40b2c8ec-ee2f-11e7-8a8f-96704f3c8caa.png width=300> |

Docs update: facebook/react-native-website#106

[ANDROID] [FEATURE] [Image] - Implement resizeMode=repeat
Closes facebook#17404

Reviewed By: achen1

Differential Revision: D7070329

Pulled By: mdvacca

fbshipit-source-id: 6a72fcbdcc7c7c2daf293dc1d8b6728f54ad0249
macdoum1 pushed a commit to macdoum1/react-native that referenced this issue Jun 28, 2018
Summary:
`<Image resizeMode="repeat" />` for Android, matching the iOS implementation (facebook#7968). (Non-goal: changing the component's API for finer-grained control / feature parity with CSS - this would be nice in the future)

As requested in e.g. facebook#14158.

Given facebook/fresco#1575, and lacking the context to follow the specific recommendations in facebook/fresco#1575 (comment), I've opted for a minimal change within RN itself.

It's likely that performance can be improved by offloading this work to Fresco in some clever way; but I'm assuming that the present naive approach is still an improvement over a userland implementation with `onLayout` and multiple `<Image>` instances.

- Picking up on a TODO note in the existing code, I implemented `MultiPostprocessor` to allow arbitrary chaining of Fresco-compatible postprocessors inside `ReactImageView`.
- Rather than extensively refactor `ImageResizeMode`, `ReactImageManager` and `ReactImageView`, I mostly preserved the existing API that maps `resizeMode` values to [`ScaleType`](http://frescolib.org/javadoc/reference/com/facebook/drawee/drawable/ScalingUtils.ScaleType.html) instances, and simply added a second mapping, to [`TileMode`](https://developer.android.com/reference/android/graphics/Shader.TileMode.html).
- To match the iOS rendering exactly for oversized images, I found that scaling with a custom `ScaleType` was required - a kind of combination of `CENTER_INSIDE` and `FIT_START` which Fresco doesn't provide - so I implemented that as `ScaleTypeStartInside`. (This is, frankly, questionable as the default behaviour on iOS to begin with - but I am aiming for parity here)
- `resizeMode="repeat"` is therefore unpacked by the view manager to the effect of:
  ```js
     view.setScaleType(ScaleTypeStartInside.INSTANCE);
     view.setTileMode(Shader.TileMode.REPEAT);
   ```
  And the added postprocessing in the view (in case of a non-`CLAMP` tile mode) consists of waiting for layout, allocating a destination bitmap and painting the source bitmap with the requested tile mode and scale type.

Note that as in facebook#17398 (comment), I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](facebook#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers.

Also, I'm happy to address any code style issues or other feedback; I'm new to this codebase and a very infrequent Android/Java coder.

Tested by enabling the relevant case in RNTester on Android.

| iOS | Android |
|-|-|
| <img src=https://user-images.githubusercontent.com/2246565/34461897-4e12008e-ee2f-11e7-8581-1dc0cc8f2779.png width=300>| <img src=https://user-images.githubusercontent.com/2246565/34461894-40b2c8ec-ee2f-11e7-8a8f-96704f3c8caa.png width=300> |

Docs update: facebook/react-native-website#106

[ANDROID] [FEATURE] [Image] - Implement resizeMode=repeat
Closes facebook#17404

Reviewed By: achen1

Differential Revision: D7070329

Pulled By: mdvacca

fbshipit-source-id: 6a72fcbdcc7c7c2daf293dc1d8b6728f54ad0249
rozele pushed a commit to microsoft/react-native-windows that referenced this issue Aug 17, 2018
Summary:
`<Image resizeMode="repeat" />` for Android, matching the iOS implementation (#7968). (Non-goal: changing the component's API for finer-grained control / feature parity with CSS - this would be nice in the future)

As requested in e.g. #14158.

Given facebook/fresco#1575, and lacking the context to follow the specific recommendations in facebook/fresco#1575 (comment), I've opted for a minimal change within RN itself.

It's likely that performance can be improved by offloading this work to Fresco in some clever way; but I'm assuming that the present naive approach is still an improvement over a userland implementation with `onLayout` and multiple `<Image>` instances.

- Picking up on a TODO note in the existing code, I implemented `MultiPostprocessor` to allow arbitrary chaining of Fresco-compatible postprocessors inside `ReactImageView`.
- Rather than extensively refactor `ImageResizeMode`, `ReactImageManager` and `ReactImageView`, I mostly preserved the existing API that maps `resizeMode` values to [`ScaleType`](http://frescolib.org/javadoc/reference/com/facebook/drawee/drawable/ScalingUtils.ScaleType.html) instances, and simply added a second mapping, to [`TileMode`](https://developer.android.com/reference/android/graphics/Shader.TileMode.html).
- To match the iOS rendering exactly for oversized images, I found that scaling with a custom `ScaleType` was required - a kind of combination of `CENTER_INSIDE` and `FIT_START` which Fresco doesn't provide - so I implemented that as `ScaleTypeStartInside`. (This is, frankly, questionable as the default behaviour on iOS to begin with - but I am aiming for parity here)
- `resizeMode="repeat"` is therefore unpacked by the view manager to the effect of:
  ```js
     view.setScaleType(ScaleTypeStartInside.INSTANCE);
     view.setTileMode(Shader.TileMode.REPEAT);
   ```
  And the added postprocessing in the view (in case of a non-`CLAMP` tile mode) consists of waiting for layout, allocating a destination bitmap and painting the source bitmap with the requested tile mode and scale type.

Note that as in facebook/react-native#17398 (comment), I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](facebook/react-native#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers.

Also, I'm happy to address any code style issues or other feedback; I'm new to this codebase and a very infrequent Android/Java coder.

Tested by enabling the relevant case in RNTester on Android.

| iOS | Android |
|-|-|
| <img src=https://user-images.githubusercontent.com/2246565/34461897-4e12008e-ee2f-11e7-8581-1dc0cc8f2779.png width=300>| <img src=https://user-images.githubusercontent.com/2246565/34461894-40b2c8ec-ee2f-11e7-8a8f-96704f3c8caa.png width=300> |

Docs update: facebook/react-native-website#106

[ANDROID] [FEATURE] [Image] - Implement resizeMode=repeat
Closes facebook/react-native#17404

Reviewed By: achen1

Differential Revision: D7070329

Pulled By: mdvacca

fbshipit-source-id: 6a72fcbdcc7c7c2daf293dc1d8b6728f54ad0249
@facebook facebook locked as resolved and limited conversation to collaborators Sep 21, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Sep 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests