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

[Android] Add DisplayP3 background and border support to View #43056

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ryanlntn
Copy link
Contributor

@ryanlntn ryanlntn commented Feb 15, 2024

Summary:

This builds on previous PRs for the wide gamut color RFC and extends Android views with support for DisplayP3 backgrounds and borders.

Changelog:

[ANDROID] [ADDED] - Add DisplayP3 background and border support to View

Test Plan:

  1. Pull changes from these previous PRs:
    [JS] Add support for CSS4 color() functions #42831
    [Android] Update ColorPropConverterto support color function values #43031
    [Android] Add isWideColorGamutEnabled to ReactActivityDelegate #43036
  2. Follow the test steps for each PR
  3. Pull these changes and build RNTester for Android: cd packages/rn-tester && yarn android
  4. Navigate to ViewExample and observe DisplayP3 background/borders

Screenshot_20240131-100112

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Infinite Red Partner: Infinite Red Partner labels Feb 15, 2024
@ryanlntn ryanlntn changed the title [Android] Add DisplayP3 background and border support to Android View [Android] Add DisplayP3 background and border support to View Feb 15, 2024
@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 Feb 15, 2024
@ryanlntn ryanlntn force-pushed the feat/wide-gamut-color-android-view branch 2 times, most recently from dbe644f to 1afc4b5 Compare March 15, 2024 10:34
@facebook-github-bot
Copy link
Contributor

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

Comment on lines 179 to 180
Method setBackgroundColorMethod = view.getClass().getMethod("setBackgroundColor", long.class);
setBackgroundColorMethod.invoke(view, backgroundColor);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we instead use something like a ColorDrawable here or avoid using reflection if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cortinico I tried making ColorDrawable work but I don't think I can make it work with long colors. Instead I think we can just check if the view is an instance of ReactViewGroup (Is this always true?) and that seems to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

we can just check if the view is an instance of ReactViewGroup (Is this always true?)

I don't think it's always true sadly

/**
* Class representing CSS spacing border colors. Modified from {@link Spacing} to support long values.
*/
public class BorderColor {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we'd want new classes to be in Kotlin at this stage

@ryanlntn ryanlntn force-pushed the feat/wide-gamut-color-android-view branch from 1afc4b5 to e5c6cb3 Compare April 17, 2024 00:27
@ryanlntn ryanlntn requested a review from cortinico April 17, 2024 01:55
@facebook-github-bot
Copy link
Contributor

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

Comment on lines 177 to 179
if (view instanceof ReactViewGroup) {
((ReactViewGroup) view).setBackgroundColor(backgroundColor);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @ryanlntn, using ReactViewGroups creates a circular dependencies in the internal CI, we have some difference in how modules are structured internally.

Can you refresh my memory and remind me why you need to cast the view to ReactViewGroup? Can't we use the same approach we use below for regular views?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cipolleschi Since I updated this to remove the use of reflection we need to cast to a type that supports setBackgroundColor(long color) or it won't compile. Regular views only support setBackgroundColor(int color). Would it be better to create a new subclass of View to extend for T?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ryanlntn We can't do in this way, unfortunately. Due to the circular dependencies, it will not work internally. And a subclass will give us no guarantees that it will be instantiated properly. I think that we need an interface.

The best way to move forward is probably:

  1. in the uimanager folder, let's create an Interface:
public interface ReactWideGamutView extends View {
  public void setBackgroundColor(long color);
  // other methods required by wide gamut
}
  1. Make the ReactViewGroup implement the ReactWideGamutView interface
  2. Replace the casting to ReactViewGroup with a check like:
if (view instanceof ReactWideGamutView) {
      ((ReactWideGamutView) view).setBackgroundColor(backgroundColor);

In this way, we don't have the circular dependency, because the ReactWideGamutView will live in the same package as ReactUIManager and we can safely cast the view to the right interface and avoid any subclass.
Also, all future subclasses of ReactViewGroup will benefit from implementing the same interface out of the box.

We should check whether there are other React componentes that we need to make them implement the same interface.... 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

This is also the same approach used for uimanager.ReactZIndexedViewGroup: as you can see this interface leaves in the uimanager package.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense to me. I'll get it updated. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

We should check whether there are other React componentes that we need to make them implement the same interface....

AFAIK this isn't the case yet. Implementing this in ReactViewGroup is all I need to get wide gamut backgrounds working for View.

@facebook-github-bot
Copy link
Contributor

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

@react-native-bot
Copy link
Collaborator

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@react-native-bot react-native-bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Oct 21, 2024
@cortinico cortinico added Never gets stale Prevent those issues and PRs from getting stale and removed Stale There has been a lack of activity on this issue and it may be closed soon. labels Oct 21, 2024
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. Never gets stale Prevent those issues and PRs from getting stale p: Infinite Red Partner: Infinite Red 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.

5 participants