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

Fix inconsistent shadows #1352

Merged
merged 4 commits into from
Aug 11, 2022
Merged

Conversation

lyzhan7
Copy link

@lyzhan7 lyzhan7 commented Aug 10, 2022

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

Pull request #1227 mostly fixed the consistency issue but had a problem with the shadow opacity. There isn't a shadowOpacity property on NSShadow, the shadow opacity from the layer needed to be passed into the alpha channel of the NSShadow shadowColor property.

Fixes #824

Test Plan

This fix was tested with the fluentui-react-native tester app.

Before:

before.mov

After:

afteropacityfix.mov

@lyzhan7 lyzhan7 requested a review from a team as a code owner August 10, 2022 23:48
@lyzhan7
Copy link
Author

lyzhan7 commented Aug 10, 2022

Note: We tried an additional approach to fix this, since the root issue behind this bug is that modifying certain layer props (including shadow related ones) on a layer owned by an NSView is undefined behavior, and we are still doing this in RCTViewManager with RCT_REMAP_VIEW_PROPERTY

RCT_REMAP_VIEW_PROPERTY(shadowColor, layer.shadowColor, CGColor)
RCT_REMAP_VIEW_PROPERTY(shadowOffset, layer.shadowOffset, CGSize)
RCT_REMAP_VIEW_PROPERTY(shadowOpacity, layer.shadowOpacity, float)
RCT_REMAP_VIEW_PROPERTY(shadowRadius, layer.shadowRadius, CGFloat)

In the second approach, rather than modifying RCTView.m, we changed RCTViewManager.m to use RCT_CUSTOM_VIEW_PROPERTY instead of RCT_REMAP_VIEW_PROPERTY, and set the view.shadow there.

MicrosoftTeams-image

We decided to keep the original approach since the result is the same, it's simpler and there will be less diffs between React Native macOS and React Native Core. If there are further issues with shadow rendering consistency this could be something we revisit.

@pull-bot
Copy link

Messages
📖

📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • macOS
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 0051dac

React/Views/RCTView.m Outdated Show resolved Hide resolved
React/Views/RCTView.m Outdated Show resolved Hide resolved
React/Views/RCTView.m Show resolved Hide resolved
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: e532f86
Branch: main

@lyzhan7 lyzhan7 merged commit 8ed4340 into microsoft:main Aug 11, 2022
@lyzhan7 lyzhan7 deleted the lyzhan-shadowconsistency branch August 11, 2022 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component shadows do not always applies in 0.63.37
4 participants