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

Implement letterSpacing on Android >= 5.0 #17398

Closed
wants to merge 10 commits into from

Conversation

motiz88
Copy link
Contributor

@motiz88 motiz88 commented Dec 31, 2017

Motivation

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 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, 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 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.

Test Plan

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

RNTester - <Text>

iOS (existing functionality, amended test) Android (new functionality & test)

RNTester - <TextInput placeholder>

iOS (not implemented, test not in this branch) Android (new functionality & test)

RNTester - <TextInput value>

iOS (not implemented, test not in this branch) Android (new functionality & test)

Related PRs

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.

Release Notes

[ANDROID] [FEATURE] [Text] - Implemented letterSpacing

@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 Dec 31, 2017
@janicduplessis
Copy link
Contributor

Thanks for the PR, going to try it in my RN fork to see if it works properly in some apps.

@hramos Could you have someone review this, not sure who to ping for android these days :S

@janicduplessis
Copy link
Contributor

janicduplessis commented Jan 20, 2018

@motiz88 Tested this and there is one major issue. Text isn't measured properly when using letterSpacing. It can be made very noticeable by adding a background color.

Letter spacing 2.0:
image

No letter spacing:
image

@motiz88
Copy link
Contributor Author

motiz88 commented Jan 20, 2018

@janicduplessis Ooh, that's going to be interesting to track down. On it, will report back soon.

@motiz88
Copy link
Contributor Author

motiz88 commented Jan 20, 2018

Can you share some code to reproduce the issue you're seeing, @janicduplessis? Trying this in RNTester (see code in 19df006) seems to render correctly.

iOS Android

Note: The bottom row isn't wrapped with flexDirection: row so gets full width. As I understand, this is per the standard RN layout algorithm for text.

@janicduplessis
Copy link
Contributor

janicduplessis commented Jan 20, 2018

@motiz88 Oh, looks like it only happens when setting fontSize + letterSpacing.

This repros it for me:

      <View style={{ flex: 1, alignItems: 'flex-start' }}>
        <Text
          style={{
            backgroundColor: 'red',
            letterSpacing: 2,
            fontSize: 12,
          }}
        >
          TEST TEST TEST
        </Text>
      </View>

@motiz88 motiz88 force-pushed the letter-spacing-android-dec2017 branch from 58611cd to 6408a25 Compare January 20, 2018 09:44
@motiz88
Copy link
Contributor Author

motiz88 commented Jan 20, 2018

Fixed in 6408a25. Apparently, spans end up being evaluated in reverse order to the one in which they're added to the ops list in buildSpannedFromShadowNode - probably due to this line increasing each subsequent one's priority, if I'm reading the code correctly. This in turn matters to CustomLetterSpacingSpan, as it must read the current font size from the TextPaint instance to perform an appropriate unit conversion.

Quite an omission in my testing approach 😨 especially as I was very much aware of the dependency between fontSize and letterSpacing. But looks good now!

iOS Android

@janicduplessis
Copy link
Contributor

@motiz88 Nice just tested again and everything seems fine! Will try to have someone from fb have a look.

@@ -49,7 +49,16 @@ const TextStylePropTypes = {
textShadowRadius: ReactPropTypes.number,
textShadowColor: ColorPropType,
/**
* @platform ios
* Increase or decrease the spacing between characters. The default value is 0, for no extra
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add these docs to the docs at https://github.com/facebook/react-native-website instead? We'll be slowly pruning out these comment blocks over the next few months.

Copy link
Contributor

Choose a reason for hiding this comment

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

(It's fine to wait for this PR to be merged first)

@hramos
Copy link
Contributor

hramos commented Jan 29, 2018

Importing for internal review.

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.

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@hramos hramos left a comment

Choose a reason for hiding this comment

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

We'll need the PR to be rebased before we can cleanly import it. Thanks.

Copy link
Contributor

@hramos hramos left a comment

Choose a reason for hiding this comment

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

Needs rebase

@hramos
Copy link
Contributor

hramos commented Feb 3, 2018

Sorry about the conflicting statements above - this still needs to be reviewed internally, but we'll need this to be rebased first in order to be able to import the PR cleanly.

@motiz88 motiz88 force-pushed the letter-spacing-android-dec2017 branch from 6408a25 to 79da4b2 Compare February 3, 2018 13:33
@motiz88
Copy link
Contributor Author

motiz88 commented Feb 3, 2018

@hramos I've rebased this on top of master as requested. UPDATE: Also reverted the commit which added the doc comments. There already is a docs PR waiting on this one: facebook/react-native-website#105

@hramos
Copy link
Contributor

hramos commented Feb 27, 2018

The PR has already been imported, which is more than can be said for most PRs. It's now waiting for review from an employee. We'll sync back here if any changes are required.

@janicduplessis
Copy link
Contributor

Thanks @motiz88 @hramos @mdvacca !! 🎉

vincentriemer pushed a commit to vincentriemer/react-native-dom that referenced this pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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
@louisiscoding
Copy link

Any news on this ?

@motiz88
Copy link
Contributor Author

motiz88 commented May 15, 2018 via email

buoyad pushed a commit to keybase/react-native that referenced this pull request 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 pull request 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
gnprice added a commit to gnprice/react-native that referenced this pull request Jun 27, 2018
Because NaN is special, the `!=` version of this condition will
always be true -- even if `mLetterSpacing` is also `Float.NaN`, which
is its default value.  It should instead be `!Float.isNaN(...)`.

The effect of the broken condition is that every text shadow node will
create a `CustomLetterSpacingSpan` around its contents, even when the
`letterSpacing` prop was never set.

Empirically, that in turn causes facebook#19126: in a controlled TextInput
after some text is first added, then deleted, further interaction
with the TextInput becomes extremely slow.  Perhaps we're somehow
ending up with large numbers of these shadow nodes (that sounds like
a bug in itself, but I guess it's normally low-impact); then this
code would have caused them each to generate more work to do.  In
any case, fixing this condition causes that issue to disappear.

The affected logic was introduced between v0.54 and v0.55, in facebook#17398
aka 5898817 "Implement letterSpacing on Android >= 5.0".

Fixes facebook#19126.
gnprice added a commit to gnprice/react-native that referenced this pull request Jun 27, 2018
Because NaN is special, the `!=` version of this condition will
always be true -- even if `mLetterSpacing` is also `Float.NaN`, which
is its default value.  It should instead be `!Float.isNaN(...)`.

The effect of the broken condition is that every text shadow node will
create a `CustomLetterSpacingSpan` around its contents, even when the
`letterSpacing` prop was never set.

Empirically, that in turn causes facebook#19126: in a controlled TextInput
after some text is first added, then deleted, further interaction
with the TextInput becomes extremely slow.  Perhaps we're somehow
ending up with large numbers of these shadow nodes (that sounds like
a bug in itself, but I guess it's normally low-impact); then this
code would have caused them each to generate more work to do.  In
any case, fixing this condition causes that issue to disappear.

The affected logic was introduced between v0.54 and v0.55, in facebook#17398
aka 5898817 "Implement letterSpacing on Android >= 5.0".

Fixes facebook#19126.
macdoum1 pushed a commit to macdoum1/react-native that referenced this pull request 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
s-nel pushed a commit to springnz/react-native that referenced this pull request Jul 5, 2018
Because NaN is special, the `!=` version of this condition will
always be true -- even if `mLetterSpacing` is also `Float.NaN`, which
is its default value.  It should instead be `!Float.isNaN(...)`.

The effect of the broken condition is that every text shadow node will
create a `CustomLetterSpacingSpan` around its contents, even when the
`letterSpacing` prop was never set.

Empirically, that in turn causes facebook#19126: in a controlled TextInput
after some text is first added, then deleted, further interaction
with the TextInput becomes extremely slow.  Perhaps we're somehow
ending up with large numbers of these shadow nodes (that sounds like
a bug in itself, but I guess it's normally low-impact); then this
code would have caused them each to generate more work to do.  In
any case, fixing this condition causes that issue to disappear.

The affected logic was introduced between v0.54 and v0.55, in facebook#17398
aka 5898817 "Implement letterSpacing on Android >= 5.0".

Fixes facebook#19126.
s-nel pushed a commit to springnz/react-native that referenced this pull request Jul 5, 2018
Because NaN is special, the `!=` version of this condition will
always be true -- even if `mLetterSpacing` is also `Float.NaN`, which
is its default value.  It should instead be `!Float.isNaN(...)`.

The effect of the broken condition is that every text shadow node will
create a `CustomLetterSpacingSpan` around its contents, even when the
`letterSpacing` prop was never set.

Empirically, that in turn causes facebook#19126: in a controlled TextInput
after some text is first added, then deleted, further interaction
with the TextInput becomes extremely slow.  Perhaps we're somehow
ending up with large numbers of these shadow nodes (that sounds like
a bug in itself, but I guess it's normally low-impact); then this
code would have caused them each to generate more work to do.  In
any case, fixing this condition causes that issue to disappear.

The affected logic was introduced between v0.54 and v0.55, in facebook#17398
aka 5898817 "Implement letterSpacing on Android >= 5.0".

Fixes facebook#19126.
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request 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
jackkav added a commit to Werkspot/react-native that referenced this pull request Oct 31, 2018
Cherry pick from: facebook@5017b86

Because NaN is special, the `!=` version of this condition will
always be true -- even if `mLetterSpacing` is also `Float.NaN`, which
is its default value.  It should instead be `!Float.isNaN(...)`.

The effect of the broken condition is that every text shadow node will
create a `CustomLetterSpacingSpan` around its contents, even when the
`letterSpacing` prop was never set.

Empirically, that in turn causes facebook#19126: in a controlled TextInput
after some text is first added, then deleted, further interaction
with the TextInput becomes extremely slow.  Perhaps we're somehow
ending up with large numbers of these shadow nodes (that sounds like
a bug in itself, but I guess it's normally low-impact); then this
code would have caused them each to generate more work to do.  In
any case, fixing this condition causes that issue to disappear.

The affected logic was introduced between v0.54 and v0.55, in facebook#17398
aka 5898817 "Implement letterSpacing on Android >= 5.0".

Fixes facebook#19126.
facebook-github-bot pushed a commit that referenced this pull request Jan 9, 2020
Summary:
This sync includes the following changes:
- **[19f6fe170](facebook/react@19f6fe170 )**: Revert "Revert "Dispatch commands to both UIManagers from both renderers (#17211)" (#17232)" (#17799) //<Eli White>//
- **[6250462be](facebook/react@6250462be )**: Renamed "ReactDOM-fb" imports to "ReactDOM" in www shims (#17797) //<Brian Vaughn>//
- **[59f21f1b2](facebook/react@59f21f1b2 )**: HostText needs to copy over from current if it is unchanged in persistent  mode (#17538) //<Sebastian Markbåge>//
- **[ccc6100d7](facebook/react@ccc6100d7 )**: Fix comments typos (#17550) //<Nick S. Plekhanov>//
- **[1b9328cd9](facebook/react@1b9328cd9 )**: Null stateNode after unmount (#17666) //<Brian Vaughn>//
- **[897976600](facebook/react@897976600 )**: [ESLint] Allow partial matches for custom Effect Hooks (#17663) //<Dan Abramov>//
- **[72592310a](facebook/react@72592310a )**: Create packages/dom-event-testing-library (#17660) //<Nicolas Gallagher>//
- **[a5e951d4c](facebook/react@a5e951d4c )**: [react-interactions] Event testing library improvements (#17614) //<Nicolas Gallagher>//
- **[7dc974542](facebook/react@7dc974542 )**: [Flight] Chunks API (#17398) //<Sebastian Markbåge>//
- **[9354dd275](facebook/react@9354dd275 )**: Make HostComponent inexact (#17412) //<Eli White>//
- **[4c270375e](facebook/react@4c270375e )**: Favor fallthrough switch instead of case statements for work tags (#17648) //<Sebastian Markbåge>//
- **[6fef7c47a](facebook/react@6fef7c47a )**: Add a regression test for switching from Fragment to a component (#17647) //<Dan Abramov>//
- **[9fe103124](facebook/react@9fe103124 )**: [react-interactions] Rename Flare APIs to deprecated and remove from RN (#17644) //<Dominic Gannaway>//
- **[4b0cdf29a](facebook/react@4b0cdf29a )**: Build FB RN targets only in experimental mode (#17641) //<Dan Abramov>//
- **[7309c5f93](facebook/react@7309c5f93 )**: Use zero-fill right shift instead of Math.floor (#17616) //<伊撒尔>//
- **[3c54df091](facebook/react@3c54df091 )**: Fix missing stacks in WWW warnings (#17638) //<Dan Abramov>//
- **[b66e86d95](facebook/react@b66e86d95 )**: [email protected] //<Dan Abramov>//
- **[c2d1561c6](facebook/react@c2d1561c6 )**: [Fast Refresh] Support injecting runtime after renderer executes (#17633) //<Dan Abramov>//
- **[f42431abe](facebook/react@f42431abe )**: Revert "Remove renderPhaseUpdates Map (#17484)" (#17623) //<Dan Abramov>//
- **[0b5a26a48](facebook/react@0b5a26a48 )**: Rename toWarnDev -> toErrorDev, toLowPriorityWarnDev -> toWarnDev (#17605) //<Dan Abramov>//
- **[0cf22a56a](facebook/react@0cf22a56a )**: Use console directly instead of warning() modules (#17599) //<Dan Abramov>//
- **[b6c423daa](facebook/react@b6c423daa )**: Use matching test command for equivalence tests (#17604) //<Dan Abramov>//
- **[8a347ed02](facebook/react@8a347ed02 )**: Remove renderPhaseUpdates Map (#17484) //<Sebastian Markbåge>//
- **[be603f5a5](facebook/react@be603f5a5 )**: [react-events] Remove lastNativeEvent in favor of SystemFlags (#17585) //<Dominic Gannaway>//
- **[b15bf3675](facebook/react@b15bf3675 )**: Add component stacks to (almost) all warnings (#17586) //<Dan Abramov>//
- **[2afeebdcc](facebook/react@2afeebdcc )**: [react-interactions] Remove responder root event types + revert commit phase change (#17577) //<Dominic Gannaway>//
- **[9ac42dd07](facebook/react@9ac42dd07 )**: Remove the condition argument from warning() (#17568) //<Laura buns>//
- **[7bf40e1cf](facebook/react@7bf40e1cf )**: Initialize update queue object on mount (#17560) //<Andrew Clark>//
- **[e039e690b](facebook/react@e039e690b )**: Revert Update Queue Refactor //<Andrew Clark>//
- **[b617db3d9](facebook/react@b617db3d9 )**: Refactor Update Queues to Fix Rebasing Bug //<Andrew Clark>//
- **[b43eec7ea](facebook/react@b43eec7ea )**: Replace `wrap-warning-with-env-check` with an eslint plugin (#17540) //<Laura buns>//
- **[acfe4b21b](facebook/react@acfe4b21b )**: [react-interactions] Upgrade passive event listeners to active listeners (#17513) //<Dominic Gannaway>//
- **[5064c7f6a](facebook/react@5064c7f6a )**: Revert Rerender Error Check (#17519) //<Sebastian Markbåge>//
- **[6d105ad3f](facebook/react@6d105ad3f )**: [react-interactions] Move Flare event registration to commit phase (#17518) //<Dominic Gannaway>//
- **[dc18b8b8d](facebook/react@dc18b8b8d )**: Don't group Idle/Offscreen work with other work (#17456) //<Sebastian Markbåge>//
- **[f523b2e0d](facebook/react@f523b2e0d )**: Use fewer global variables in Hooks (#17480) //<Sebastian Markbåge>//
- **[d75323f65](facebook/react@d75323f65 )**: Remove case that only exists for createBatch (#17506) //<Sebastian Markbåge>//
- **[79572e34d](facebook/react@79572e34d )**: Adjust SuspenseList CPU bound heuristic (#17455) //<Sebastian Markbåge>//
- **[969f4b5bb](facebook/react@969f4b5bb )**: Change DevTools hook warning message (#17478) //<Dan Abramov>//
- **[6470e0f16](facebook/react@6470e0f16 )**: [Fresh] Make all errors recoverable (#17438) //<Dan Abramov>//

Changelog:
[General][Changed] - React sync for revisions 6cff70a...19f6fe1

Reviewed By: TheSavior

Differential Revision: D19318286

fbshipit-source-id: acaa5224f7162a274c395a62e54da82199001005
osdnk pushed a commit to osdnk/react-native that referenced this pull request Mar 9, 2020
Summary:
This sync includes the following changes:
- **[19f6fe170](facebook/react@19f6fe170 )**: Revert "Revert "Dispatch commands to both UIManagers from both renderers (facebook#17211)" (facebook#17232)" (facebook#17799) //<Eli White>//
- **[6250462be](facebook/react@6250462be )**: Renamed "ReactDOM-fb" imports to "ReactDOM" in www shims (facebook#17797) //<Brian Vaughn>//
- **[59f21f1b2](facebook/react@59f21f1b2 )**: HostText needs to copy over from current if it is unchanged in persistent  mode (facebook#17538) //<Sebastian Markbåge>//
- **[ccc6100d7](facebook/react@ccc6100d7 )**: Fix comments typos (facebook#17550) //<Nick S. Plekhanov>//
- **[1b9328cd9](facebook/react@1b9328cd9 )**: Null stateNode after unmount (facebook#17666) //<Brian Vaughn>//
- **[897976600](facebook/react@897976600 )**: [ESLint] Allow partial matches for custom Effect Hooks (facebook#17663) //<Dan Abramov>//
- **[72592310a](facebook/react@72592310a )**: Create packages/dom-event-testing-library (facebook#17660) //<Nicolas Gallagher>//
- **[a5e951d4c](facebook/react@a5e951d4c )**: [react-interactions] Event testing library improvements (facebook#17614) //<Nicolas Gallagher>//
- **[7dc974542](facebook/react@7dc974542 )**: [Flight] Chunks API (facebook#17398) //<Sebastian Markbåge>//
- **[9354dd275](facebook/react@9354dd275 )**: Make HostComponent inexact (facebook#17412) //<Eli White>//
- **[4c270375e](facebook/react@4c270375e )**: Favor fallthrough switch instead of case statements for work tags (facebook#17648) //<Sebastian Markbåge>//
- **[6fef7c47a](facebook/react@6fef7c47a )**: Add a regression test for switching from Fragment to a component (facebook#17647) //<Dan Abramov>//
- **[9fe103124](facebook/react@9fe103124 )**: [react-interactions] Rename Flare APIs to deprecated and remove from RN (facebook#17644) //<Dominic Gannaway>//
- **[4b0cdf29a](facebook/react@4b0cdf29a )**: Build FB RN targets only in experimental mode (facebook#17641) //<Dan Abramov>//
- **[7309c5f93](facebook/react@7309c5f93 )**: Use zero-fill right shift instead of Math.floor (facebook#17616) //<伊撒尔>//
- **[3c54df091](facebook/react@3c54df091 )**: Fix missing stacks in WWW warnings (facebook#17638) //<Dan Abramov>//
- **[b66e86d95](facebook/react@b66e86d95 )**: [email protected] //<Dan Abramov>//
- **[c2d1561c6](facebook/react@c2d1561c6 )**: [Fast Refresh] Support injecting runtime after renderer executes (facebook#17633) //<Dan Abramov>//
- **[f42431abe](facebook/react@f42431abe )**: Revert "Remove renderPhaseUpdates Map (facebook#17484)" (facebook#17623) //<Dan Abramov>//
- **[0b5a26a48](facebook/react@0b5a26a48 )**: Rename toWarnDev -> toErrorDev, toLowPriorityWarnDev -> toWarnDev (facebook#17605) //<Dan Abramov>//
- **[0cf22a56a](facebook/react@0cf22a56a )**: Use console directly instead of warning() modules (facebook#17599) //<Dan Abramov>//
- **[b6c423daa](facebook/react@b6c423daa )**: Use matching test command for equivalence tests (facebook#17604) //<Dan Abramov>//
- **[8a347ed02](facebook/react@8a347ed02 )**: Remove renderPhaseUpdates Map (facebook#17484) //<Sebastian Markbåge>//
- **[be603f5a5](facebook/react@be603f5a5 )**: [react-events] Remove lastNativeEvent in favor of SystemFlags (facebook#17585) //<Dominic Gannaway>//
- **[b15bf3675](facebook/react@b15bf3675 )**: Add component stacks to (almost) all warnings (facebook#17586) //<Dan Abramov>//
- **[2afeebdcc](facebook/react@2afeebdcc )**: [react-interactions] Remove responder root event types + revert commit phase change (facebook#17577) //<Dominic Gannaway>//
- **[9ac42dd07](facebook/react@9ac42dd07 )**: Remove the condition argument from warning() (facebook#17568) //<Laura buns>//
- **[7bf40e1cf](facebook/react@7bf40e1cf )**: Initialize update queue object on mount (facebook#17560) //<Andrew Clark>//
- **[e039e690b](facebook/react@e039e690b )**: Revert Update Queue Refactor //<Andrew Clark>//
- **[b617db3d9](facebook/react@b617db3d9 )**: Refactor Update Queues to Fix Rebasing Bug //<Andrew Clark>//
- **[b43eec7ea](facebook/react@b43eec7ea )**: Replace `wrap-warning-with-env-check` with an eslint plugin (facebook#17540) //<Laura buns>//
- **[acfe4b21b](facebook/react@acfe4b21b )**: [react-interactions] Upgrade passive event listeners to active listeners (facebook#17513) //<Dominic Gannaway>//
- **[5064c7f6a](facebook/react@5064c7f6a )**: Revert Rerender Error Check (facebook#17519) //<Sebastian Markbåge>//
- **[6d105ad3f](facebook/react@6d105ad3f )**: [react-interactions] Move Flare event registration to commit phase (facebook#17518) //<Dominic Gannaway>//
- **[dc18b8b8d](facebook/react@dc18b8b8d )**: Don't group Idle/Offscreen work with other work (facebook#17456) //<Sebastian Markbåge>//
- **[f523b2e0d](facebook/react@f523b2e0d )**: Use fewer global variables in Hooks (facebook#17480) //<Sebastian Markbåge>//
- **[d75323f65](facebook/react@d75323f65 )**: Remove case that only exists for createBatch (facebook#17506) //<Sebastian Markbåge>//
- **[79572e34d](facebook/react@79572e34d )**: Adjust SuspenseList CPU bound heuristic (facebook#17455) //<Sebastian Markbåge>//
- **[969f4b5bb](facebook/react@969f4b5bb )**: Change DevTools hook warning message (facebook#17478) //<Dan Abramov>//
- **[6470e0f16](facebook/react@6470e0f16 )**: [Fresh] Make all errors recoverable (facebook#17438) //<Dan Abramov>//

Changelog:
[General][Changed] - React sync for revisions 6cff70a...19f6fe1

Reviewed By: TheSavior

Differential Revision: D19318286

fbshipit-source-id: acaa5224f7162a274c395a62e54da82199001005
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants