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

[macOS][Xcode 15] Avoid using dirtyRect in drawRect: #2136

Merged
merged 1 commit into from Sep 20, 2023

Conversation

Saadnajmi
Copy link
Contributor

@Saadnajmi Saadnajmi commented Sep 15, 2023

Note: This doesn't 100% fix macOS SVG rendering with Xcode 15, but gets up 90% of the way there 🙏

Summary

See:

Apple made a breaking change in Xcode 15 / macOS Sonoma which breaks usages of drawRect:. You can no longer trust that the OS will provide you a dirtyRect will be within the bounds of your view. Specifically (from the appkit release notes):

Filling the dirty rect of a view inside of -drawRect. A fairly common pattern is to simply rect fill the dirty rect passed into an override of NSView.draw(). The dirty rect can now extend outside of your view’s bounds. This pattern can be adjusted by filling the bounds instead of the dirty rect, or by setting clipsToBounds = true.

This led to an unfortunate bug where any SVG drawn took up the full width/height of your window. Let's follow Apple's advice and draw using [self bounds] instead of dirtyRect.

Test Plan

I built our FluentUI React Native test app with and without the one line change on Xcode 15. Notice how without the change, the chevron that is otherwise in the top right dropdown takes up the whole window.

Before:
Screenshot 2023-09-15 at 1 37 32 PM

After:
Screenshot 2023-09-15 at 1 47 25 PM

Checklist

  • I have tested this on a device and a simulator
  • I added documentation in README.md
  • I updated the typed files (typescript)
  • I added a test for the API in the __tests__ folder

@Saadnajmi
Copy link
Contributor Author

Probably worth sanity checking both macOS and iOS test apps in this repo, but I wanted to get this review out ASAP as we're close to macOS Sonoma release

@Saadnajmi
Copy link
Contributor Author

Testing against the example in the repo, it seems Images still render weird :(

image

@WoLewicki
Copy link
Member

@Saadnajmi so currently it still does not work correctly in case of Image component? Did you spot any other issues?

@Saadnajmi
Copy link
Contributor Author

@Saadnajmi so currently it still does not work correctly in case of Image component? Did you spot any other issues?

No, just that one. I'll spend today trying to figure that one out but given that macOS Sonoma / Xcode 15 is impending, I may just ask to go with the one line change so we fix the 90% case 😅

@Saadnajmi
Copy link
Contributor Author

Saadnajmi commented Sep 20, 2023

@WoLewicki It looks like there will be some deeper debugging needed to understand just how much breaks with Xcode 15, as it seems more stuff related to clipping is broken :/. In the meantime, can we merge this change as it doesn't seem to affect iOS and gets us most of the way there?

iOS screenshots:
Screenshot 2023-09-20 at 12 27 58 AM
Screenshot 2023-09-20 at 12 28 02 AM

@WoLewicki WoLewicki merged commit 23d65b9 into software-mansion:main Sep 20, 2023
4 checks passed
@@ -293,7 +293,7 @@ - (void)drawRect:(CGRect)rect
_boundingBox = rect;

Choose a reason for hiding this comment

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

Is this the issue? Shouldn't this be set to [self bounds]?

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 didn't seem to have any effect in my testing.

The remaining issue I found is that SVG images will still render with a large width/height and not properly clip. Tracing the callstack , I got the issue to to this method in RNSVGGroups.mm:

- (void)setupGlyphContext:(CGContextRef)context
{
  CGRect clipBounds = CGContextGetClipBoundingBox(context); <----
  ....
}

CGContextGetClipBoundingBox returns an abnormally large box. My next step is to compare what it returns when compiled against Xcode 14/macOS 13

Choose a reason for hiding this comment

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

Did you ever figure out what was going on for your case? We were seeing a similar issue where SVGImages weren't rendering properly, except for us CGContextGetClipBoundingBox at that point was abnormally small, with width and height of 1.

I'm not too familiar with Apple APIs, but the farthest I got was noticing that the abnormal context came from the call to UIGraphicsGetCurrentContext. Before I could investigate further, we suddenly started getting reasonable bounding boxes and I was no longer able to reproduce 😕.

Copy link
Contributor Author

@Saadnajmi Saadnajmi Oct 27, 2023

Choose a reason for hiding this comment

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

I did not get any further. Might be worth filing a Github issue to track. I'm curious what your use case is?

Copy link

Choose a reason for hiding this comment

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

We're loading an image and then drawing annotations that a user specified using SVGs, basically.

I'll try to file another issue once we start seeing it again, but it unfortunately (fortunately?) seems to work on my machine™.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, the method you linked is a UIKit (aka, iOS only) method we shim on macOS: https://github.com/microsoft/react-native-macos/blob/2fe53e8050a9c0b0906c0e092764ffdebcc039d4/packages/react-native/React/Base/macOS/RCTUIKit.m#L29-L32

I've been meaning to revisit some of these shims, but ultimately I think it would call the same underlying NSGraphicsContext code. I couldn't find any apple docs about CGContextGetClipBoundingBox changing, but It very much feels like this method also had a breaking change with Xcode 15 / the macOS 14 SDK.

renovate bot added a commit to valora-inc/wallet that referenced this pull request Oct 19, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[react-native-svg](https://togithub.com/react-native-community/react-native-svg)
| [`^13.9.0` ->
`^13.14.0`](https://renovatebot.com/diffs/npm/react-native-svg/13.9.0/13.14.0)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/react-native-svg/13.14.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/react-native-svg/13.14.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/react-native-svg/13.9.0/13.14.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/react-native-svg/13.9.0/13.14.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>react-native-community/react-native-svg
(react-native-svg)</summary>

###
[`v13.14.0`](https://togithub.com/software-mansion/react-native-svg/releases/tag/v13.14.0)

[Compare
Source](https://togithub.com/react-native-community/react-native-svg/compare/v13.13.0...v13.14.0)

PR adding improvements to the library. Thanks for all your contributions
🚀

#### What's Changed

- Create FUNDING.yml by [@&#8203;aleqsio](https://togithub.com/aleqsio)
in
[software-mansion/react-native-svg#2133
- docs: Update deprecated expo install instructions to \`npx expo
install´ by
[@&#8203;GabrieldosSantosOliveira](https://togithub.com/GabrieldosSantosOliveira)
in
[software-mansion/react-native-svg#2128
- \[macOS]\[Xcode 15] Avoid using dirtyRect in `drawRect:` by
[@&#8203;Saadnajmi](https://togithub.com/Saadnajmi) in
[software-mansion/react-native-svg#2136
- fix: error for transform on web by
[@&#8203;WoLewicki](https://togithub.com/WoLewicki) in
[software-mansion/react-native-svg#2139
- chore(deps): bump activesupport from 6.1.7.4 to 7.0.7.2 in
/TestsExample by [@&#8203;dependabot](https://togithub.com/dependabot)
in
[software-mansion/react-native-svg#2122
- fix: web not working with reanimated in example by
[@&#8203;WoLewicki](https://togithub.com/WoLewicki) in
[software-mansion/react-native-svg#2140

#### New Contributors

- [@&#8203;aleqsio](https://togithub.com/aleqsio) made their first
contribution in
[software-mansion/react-native-svg#2133
-
[@&#8203;GabrieldosSantosOliveira](https://togithub.com/GabrieldosSantosOliveira)
made their first contribution in
[software-mansion/react-native-svg#2128
- [@&#8203;Saadnajmi](https://togithub.com/Saadnajmi) made their first
contribution in
[software-mansion/react-native-svg#2136

**Full Changelog**:
software-mansion/react-native-svg@v13.13.0...v13.14.0

###
[`v13.13.0`](https://togithub.com/software-mansion/react-native-svg/releases/tag/v13.13.0)

[Compare
Source](https://togithub.com/react-native-community/react-native-svg/compare/v13.12.0...v13.13.0)

Minor release restoring usage of
`UIGraphicsBeginImageContextWithOptions` on `macOS` since there is no
implementation for `UIGraphicsImageRendererFormat` there yet.

#### What's Changed

- feat: add macos back to Example app by
[@&#8203;WoLewicki](https://togithub.com/WoLewicki) in
[software-mansion/react-native-svg#2119

**Full Changelog**:
software-mansion/react-native-svg@v13.12.0...v13.13.0

###
[`v13.12.0`](https://togithub.com/software-mansion/react-native-svg/releases/tag/v13.12.0)

[Compare
Source](https://togithub.com/react-native-community/react-native-svg/compare/v13.11.0...v13.12.0)

Minor release introducing the change of API used on `iOS` from
[UIGraphicsBeginImageContextWithOptions](https://developer.apple.com/documentation/uikit/1623912-uigraphicsbeginimagecontextwitho)
to
[UIGraphicsImageRenderer](https://developer.apple.com/documentation/uikit/uigraphicsimagerenderer).

#### What's Changed

- feat: remove UIGraphicsBeginImageContextWithOptions from repo by
[@&#8203;WoLewicki](https://togithub.com/WoLewicki) in
[software-mansion/react-native-svg#2117

**Full Changelog**:
software-mansion/react-native-svg@v13.11.0...v13.12.0

###
[`v13.11.0`](https://togithub.com/software-mansion/react-native-svg/releases/tag/v13.11.0)

[Compare
Source](https://togithub.com/react-native-community/react-native-svg/compare/v13.10.0...v13.11.0)

Minor release cleaning the repository 🧹 Please submit any issues that
come up with the newest version 🐛 Thanks for your contributions 🚀

#### What's Changed

- fix: make web platform types compatible with native types by
[@&#8203;nderscore](https://togithub.com/nderscore) in
[software-mansion/react-native-svg#2091
- docs: update usage.md for react-native 0.72 by
[@&#8203;Letty](https://togithub.com/Letty) in
[software-mansion/react-native-svg#2104
- docs: update README.md by [@&#8203;Mhp23](https://togithub.com/Mhp23)
in
[software-mansion/react-native-svg#2110
- fix: bump packages, eslint, tsconfig, prettier and resolve all
conflicts by [@&#8203;WoLewicki](https://togithub.com/WoLewicki) in
[software-mansion/react-native-svg#2114

#### New Contributors

- [@&#8203;nderscore](https://togithub.com/nderscore) made their first
contribution in
[software-mansion/react-native-svg#2091
- [@&#8203;Letty](https://togithub.com/Letty) made their first
contribution in
[software-mansion/react-native-svg#2104
- [@&#8203;Mhp23](https://togithub.com/Mhp23) made their first
contribution in
[software-mansion/react-native-svg#2110

**Full Changelog**:
software-mansion/react-native-svg@v13.10.0...v13.11.0

###
[`v13.10.0`](https://togithub.com/software-mansion/react-native-svg/releases/tag/v13.10.0)

[Compare
Source](https://togithub.com/react-native-community/react-native-svg/compare/v13.9.0...v13.10.0)

Minor release fixing some long-standing issues, adding new features such
as web support for `toDataURL`
([software-mansion/react-native-svg#2072)
and `fallback` prop for `SvgXml` and others
([software-mansion/react-native-svg#2071).
It also adds support for RN 0.72 on **new architecture**, at the same
time dropping the support for RN 0.71 there. Thanks for all your
contributions 🚀

#### What's Changed

- Fix compilation errors on Windows by
[@&#8203;christophpurrer](https://togithub.com/christophpurrer) in
[software-mansion/react-native-svg#2045
- Fix setNativeProps type by
[@&#8203;fauri13](https://togithub.com/fauri13) in
[software-mansion/react-native-svg#2058
- fix: remove deprecated import from react-native-web by
[@&#8203;janlat](https://togithub.com/janlat) in
[software-mansion/react-native-svg#2027
- fix: Updating iOS version
[#&#8203;2038](https://togithub.com/react-native-community/react-native-svg/issues/2038)
by [@&#8203;sgabriel](https://togithub.com/sgabriel) in
[software-mansion/react-native-svg#2041
- Fix syntax error in RNSVGImageShadowNode::initialStateData by
[@&#8203;hsjoberg](https://togithub.com/hsjoberg) in
[software-mansion/react-native-svg#2079
- Fix
[#&#8203;1345](https://togithub.com/react-native-community/react-native-svg/issues/1345):
opacity does not work with currentColor on Android by
[@&#8203;laptou](https://togithub.com/laptou) in
[software-mansion/react-native-svg#2080
- chore: bump Example to 0.72 by
[@&#8203;WoLewicki](https://togithub.com/WoLewicki) in
[software-mansion/react-native-svg#2084
- chore(deps): bump fast-xml-parser from 4.1.3 to 4.2.4 in /TestsExample
by [@&#8203;dependabot](https://togithub.com/dependabot) in
[software-mansion/react-native-svg#2063
- chore(deps): bump vm2 from 3.9.14 to 3.9.19 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[software-mansion/react-native-svg#2056
- chore(deps): bump fast-xml-parser from 4.2.4 to 4.2.5 in /TestsExample
by [@&#8203;dependabot](https://togithub.com/dependabot) in
[software-mansion/react-native-svg#2085
- Add web support for toDataURL() on svg tags by
[@&#8203;chrispader](https://togithub.com/chrispader) in
[software-mansion/react-native-svg#2072
- Prevents SvgUri crashes when uri is invalid and adds fallback prop by
[@&#8203;PiotrWszolek](https://togithub.com/PiotrWszolek) in
[software-mansion/react-native-svg#2071
- chore: run prettier and lint by
[@&#8203;WoLewicki](https://togithub.com/WoLewicki) in
[software-mansion/react-native-svg#2087
- chore: bump TestsExample to 0.72 by
[@&#8203;WoLewicki](https://togithub.com/WoLewicki) in
[software-mansion/react-native-svg#2088
- feat: strokeDasharray with Animated by
[@&#8203;WoLewicki](https://togithub.com/WoLewicki) in
[software-mansion/react-native-svg#2089

#### New Contributors

- [@&#8203;christophpurrer](https://togithub.com/christophpurrer) made
their first contribution in
[software-mansion/react-native-svg#2045
- [@&#8203;fauri13](https://togithub.com/fauri13) made their first
contribution in
[software-mansion/react-native-svg#2058
- [@&#8203;janlat](https://togithub.com/janlat) made their first
contribution in
[software-mansion/react-native-svg#2027
- [@&#8203;sgabriel](https://togithub.com/sgabriel) made their first
contribution in
[software-mansion/react-native-svg#2041
- [@&#8203;laptou](https://togithub.com/laptou) made their first
contribution in
[software-mansion/react-native-svg#2080
- [@&#8203;chrispader](https://togithub.com/chrispader) made their first
contribution in
[software-mansion/react-native-svg#2072
- [@&#8203;PiotrWszolek](https://togithub.com/PiotrWszolek) made their
first contribution in
[software-mansion/react-native-svg#2071

**Full Changelog**:
software-mansion/react-native-svg@v13.9.0...v13.10.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 5pm,every weekend" in timezone
America/Los_Angeles, Automerge - "after 5pm,every weekend" in timezone
America/Los_Angeles.

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/valora-inc/wallet).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xOS4yIiwidXBkYXRlZEluVmVyIjoiMzcuMTkuMiIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: valora-bot <valorabot@valoraapp.com>
@Saadnajmi Saadnajmi deleted the patch-1 branch October 27, 2023 19:34
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.

None yet

4 participants