Skip to content

feat(android): background-size, position and repeat styles#52282

Closed
intergalacticspacehighway wants to merge 11 commits into
react:mainfrom
intergalacticspacehighway:feat/background-styles-android
Closed

feat(android): background-size, position and repeat styles#52282
intergalacticspacehighway wants to merge 11 commits into
react:mainfrom
intergalacticspacehighway:feat/background-styles-android

Conversation

@intergalacticspacehighway

@intergalacticspacehighway intergalacticspacehighway commented Jun 26, 2025

Copy link
Copy Markdown
Contributor

Summary:

This PR adds support for background size, position and repeat styles. It follows the CSS spec. Currently we default to background-origin: padding-box and background-clip : border-box to match the web's behavior. We can introduce these styles later. I have split the PR intro three parts for review. This PR includes android only changes. I wanted to introduce one style at a time, but CSS spec is such that size, position and repeat are intertwined.

Changelog:

[ANDROID][ADDED] - Background size, position and repeat styles.

Test Plan:

Merge the JS PR, rebuild android app and test RNTester app, it includes BackgroundImageExample. I have also added testcases for parsing syntax in JS.

Screen.Recording.2025-06-26.at.10.47.17.AM.mp4

null checks
@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 Jun 26, 2025
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jun 26, 2025
view: View,
backgroundRepeats: List<BackgroundRepeat>?
): Unit {
if (ReactNativeFeatureFlags.enableNewBackgroundAndBorderDrawables()) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Only enabled for new drawables as it is true by default now.

public companion object {
@JvmStatic
public fun setFromDynamic(dynamic: Dynamic): LengthPercentage? {
public fun setFromDynamic(dynamic: Dynamic, allowNegative: Boolean = false): LengthPercentage? {

@intergalacticspacehighway intergalacticspacehighway Jun 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Did not like this change, but we need to support negative values for background-position. lmk if I should take some other approach!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is fine.. I'll think about it a bit

@cortinico cortinico added p: Expo Partner: Expo Partner labels Sep 10, 2025
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@jorge-cab has imported this pull request. If you are a Meta employee, you can view this in D82993837.

@jorge-cab jorge-cab left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Logic and functionality seem good! My main concern is that we are bloating BackgroundDrawable quite a bit with something that should realistically be a different layer.

I think this should just be another drawable BackgroundImageDrawable and should be set between the BackgroundDrawable and BorderDrawable in the CompositeBackgroundDrawable class.

Recently we just deleted CSSBackgroundDrawable which was combining both the Background and Border layers, which took an annoying amount of time so I would like to avoid that situation as much as possible.

Specially once we start looking into adding background-image which will require even more logic and integration with an Image pipeline

canvas.save()
// 1. create a clipping path that matches the border radius.
val clipPath = Path()
val cornerRadii =

@jorge-cab jorge-cab Sep 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Using paths to draw is slower than using drawRoundRect or drawRect which is why before we only used paths when necessary. I think this should be something you can do here?

Overall I don't think we should remove the original code, just expand another conditional for when we have size/repeat/position set, but maybe I'm missing something

@intergalacticspacehighway intergalacticspacehighway Sep 29, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Simplified it a lot in last commits. We just need Path to clip the drawable. e.g. on background repeat: repeat we have to repeat gradients that go out of bounds, so need to clip them (on iOS we handle it using maskToBounds))

Comment on lines +74 to +77
public enum class BackgroundSizeKeyword {
Cover,
Contain
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I get this correctly we are no-oping on Cover and Contain since gradients don't use them, maybe we shouldn't create a class and complicate the logic to accomodate them?

That would also simplify BackgroundSize quite a bit and we can deal with this complexity once we actually need to implement it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree. Done.

NoRepeat
}

public class BackgroundRepeat(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think for this and most other classes you added we should make them private unless you have a clear reason to make them public

@intergalacticspacehighway intergalacticspacehighway Sep 29, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah, we need them in BackgroundStyleApplicator so need to keep them public i guess. I think most styles are public right now. e.g. OutlineStyle

public companion object {
@JvmStatic
public fun setFromDynamic(dynamic: Dynamic): LengthPercentage? {
public fun setFromDynamic(dynamic: Dynamic, allowNegative: Boolean = false): LengthPercentage? {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is fine.. I'll think about it a bit

@jorge-cab

Copy link
Copy Markdown
Contributor

Position also looks off in Android but in a different way than iOS (#52283 (comment))
Android:
Screenshot 2025-09-26 at 10 22 53 AM

style={{
  width: 200,
  height: 200,
  experimental_backgroundImage:
    'linear-gradient(45deg, #ff6b6b 25%, transparent 25%, transparent 75%, #4ecdc4 75%)',
  experimental_backgroundRepeat: 'space',
  experimental_backgroundSize: '33.33px 66.67px',
  experimental_backgroundPosition: '16.5px 8.25px',
  backgroundColor: 'white',
  borderWidth: 2,
  borderColor: 'purple',
}}

Web:
Screenshot 2025-09-26 at 10 17 14 AM

  style="
      width: 200px;
      height: 200px;
      background-image: linear-gradient(
          45deg,
          #ff6b6b 25%,
          transparent 25%,
          transparent 75%,
          #4ecdc4 75%
      );
      background-repeat: space;
      background-size: 33.33px 66.67px;
      background-position: 16.5px 8.25px;
      background-color: white;
      border: 2px solid purple;
  "

@intergalacticspacehighway

intergalacticspacehighway commented Sep 29, 2025

Copy link
Copy Markdown
Contributor Author

@jorge-cab thanks for catching the issue. There was rounding pixel error on android and iOS was accounting background position for repeat space styling. I have pushed fixes on iOS and Android, also added the above example in JS PR.

simulator_screenshot_282529FD-C365-437F-BD66-B56FB61E1F33

Background Image drawable

Makes a lot of sense. Just added Background Image drawable and cleaned up Background drawable. Maybe later we can rename Background drawable to Background color drawable.

@jorge-cab jorge-cab left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Clean! I'll have this go through internal review and will update here with any other requested changes. Thank you for working on this!

…react/uimanager/drawable/BackgroundImageDrawable.kt

Co-authored-by: Jorge Cabiedes <57368278+jorge-cab@users.noreply.github.com>
…react/uimanager/drawable/BackgroundImageDrawable.kt

Co-authored-by: Jorge Cabiedes <57368278+jorge-cab@users.noreply.github.com>
@intergalacticspacehighway

Copy link
Copy Markdown
Contributor Author

The e2e tests passes locally. Looks like a timeout failure maybe.

Screenshot 2025-10-09 at 5 34 13 PM

@cortinico cortinico left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review automatically exported from Phabricator review in Meta.

@meta-codesync

meta-codesync Bot commented Oct 22, 2025

Copy link
Copy Markdown

@jorge-cab has imported this pull request. If you are a Meta employee, you can view this in D82993837.

@intergalacticspacehighway

intergalacticspacehighway commented Oct 23, 2025

Copy link
Copy Markdown
Contributor Author

@jorge-cab we need to run e2e after merging this PR with JS PR. I checked the e2e failing artifact. It has below error, it gets fixed when we add the JS changes. Also i tested radial/linear gradients on API level 24 emulator (with the JS changes). Seems to be working for me. lmk if i am missing something!

Screenshot 2025-10-23 at 9 51 45 AM

@meta-codesync meta-codesync Bot closed this in e859293 Oct 24, 2025
meta-codesync Bot pushed a commit that referenced this pull request Oct 24, 2025
Summary:
This PR adds support for background size, position and repeat styles. It follows the [CSS](https://www.w3.org/TR/css-backgrounds-3/#backgrounds) spec. Currently we default to `background-origin: padding-box` and `background-clip : border-box` to match the web's behavior. We can introduce these styles later. I have split the PR intro three parts for review. This PR includes JS parsing and style propagation to native changes. I wanted to introduce one style at a time, but CSS spec is such that size, position and repeat are intertwined.

## Changelog:
[GENERAL][ADDED] - Background size, position and repeat styles.
<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: #52284

Test Plan:
Merge the [iOS](#52283) and [android](#52282) PR into this, this PR includes `BackgroundImageExample`. I have also added testcases for parsing syntax in JS.

https://github.com/user-attachments/assets/b7192fdf-52ba-4eb0-a1be-d47c72d87e92

Reviewed By: joevilches

Differential Revision: D82973282

Pulled By: jorge-cab

fbshipit-source-id: a94e33962c6708be963e1cac049da50d4764da64
@meta-codesync

meta-codesync Bot commented Oct 24, 2025

Copy link
Copy Markdown

@jorge-cab merged this pull request in e859293.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Oct 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants