Skip to content

Allow touches on overflowed children when overflow: visible (Android) - #26374

Closed
danielgindi wants to merge 59 commits into
react:mainfrom
danielgindi:feature/android_clip_touches
Closed

Allow touches on overflowed children when overflow: visible (Android)#26374
danielgindi wants to merge 59 commits into
react:mainfrom
danielgindi:feature/android_clip_touches

Conversation

@danielgindi

@danielgindidanielgindi commented Sep 8, 2019

Copy link
Copy Markdown
Contributor

Summary

Currently overflow: visible allows showing child views that are outside the parent's bounds (well, overflow: visible on the grandparent allows that, not the parent).
But those non-clipped children do not receive touches.

This is an attempt to solve it.

Changelog

[Android] [Fixed] - overflow: visible now allows interaction with children that are rendered outside their parents' bounds.

Test Plan

  • Added overflow: visible, tapped on a button.
  • Added a MapView, tested scrolling the map while overflowed elements are visible over it, and tapping on those elements.

@facebook-github-botfacebook-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 Sep 8, 2019
@react-native-botreact-native-bot added Bug Platform: Android Android applications. labels Sep 8, 2019
@danielgindi

Copy link
Copy Markdown
ContributorAuthor

For some reason the CI fails, without any details.

@danielgindi
danielgindiforce-pushed the feature/android_clip_touches branch from 8655e56 to 5b06654CompareOctober 4, 2019 06:57
danielgindi referenced this pull request Oct 27, 2019
Summary:
Adds support for the `overflow` style property on React Native for Android.
This is the second attempt to do this. See 6110a4c (D8666509) for the first attempt.
Similar to the first attempt, this sets `setClipChildren(false)` by default on all `ViewGroup` instances. However, this differs in how it implements `overflow: hidden`. Instead of conditionally setting `setClipChildren`, this manually clips children to the `ViewGroup`'s bounds (which was incidentally what we were doing for background + border radius already).
Reviewed By: achen1
Differential Revision: D8690805
fbshipit-source-id: 58757825cd9d138c18c8758918d85b4ca1915f87
@nick-michael

nick-michael commented Nov 16, 2019

Copy link
Copy Markdown

Can anyone trigger a rebuild of this? Maybe was just a bad day for CI?
@danielgindi if you could just force-push the same commit I guess that would trigger

@danielgindi

danielgindi commented Nov 16, 2019

Copy link
Copy Markdown
ContributorAuthor

@nick-michael Rebasing on latest master

@danielgindi
danielgindiforce-pushed the feature/android_clip_touches branch from ac2a12f to 9a0154fCompareNovember 16, 2019 18:09
@danielgindi

Copy link
Copy Markdown
ContributorAuthor

I'm guessing another bad day for CI?

@nick-michael

Copy link
Copy Markdown

Haha guess not, I don't want this to get forgotten about though, this is such a welcome change for RN android

@ltcaosj

Copy link
Copy Markdown

We really need to PR merged :). Same stuff is working fine on iOS, not on Android.

@kdenz

Copy link
Copy Markdown

Yes we'll need this urgently too, because we have an expandable button which will go overflow and open an animated view action menu, and right now on android users cannot interact with the expanded content directly. Changing pointerEvents and zIndex doesn't work.

@ltcaosj

Copy link
Copy Markdown

@danielgindi What should we do to resolve the CI issue? Your fixing is so much import to us and the react-native community. After fixing this one we can create a complex UI much easier.

@danielgindi

danielgindi commented Dec 1, 2019

Copy link
Copy Markdown
ContributorAuthor

@ltcaosj I have no idea. RN does not compile easily, and I can't spend too much time forcing it to and then troubleshooting their CI scripts. I did not even touch the iOS code, but the CI is failing on iOS. So I'm not going to even try to solve this, as it's not my issue.

Edit: Not trying to be negative here. Just a bit (a lot?) frustrated by unrelated errors being thrown all around for absolutely no reason. I see that this one is important to a lot of people, so if anyone has a few hours to spare, please do try to look into it. I will try also but I do not have a lot of free time these days.

@fabOnReact

fabOnReact commented Dec 31, 2019

Copy link
Copy Markdown
Contributor

ci/circleci: js_coverage — Your tests failed on CircleCI

#!/bin/bash -eo pipefail
yarn test --coverage --maxWorkers=2
cat ./coverage/lcov.info | ./node_modules/.bin/coveralls

All test succed,

Test Suites: 102 passed, 102 total
Tests: 1 skipped, 981 passed, 982 total
Snapshots: 477 passed, 477 total
Time: 94.842s
Ran all test suites.
Done in 95.74s.

but command /home/circleci/react-native/node_modules/coveralls/bin/coveralls.js fails

/home/circleci/react-native/node_modules/coveralls/bin/coveralls.js:18
throw err;
^
Bad response: 422 {"message":"Couldn't find a repository matching this job.","error":true}
Exited with code 1

this errors is connected to misconfiguration inside circleci of the coveralls js library.

A quick search online makes me think about the following issues:

  1. webserver IPs accidentally blacklisted by CircleCI
  2. log in and out of Coveralls.io with my GitHub account a couple of times and disable/re-enable the GitHub apps integration.
  3. add empty .coveralls.yml file to repo and push
  4. Coveralls returns 422 error: Couldn't find a repository matching this job lemurheavy/coveralls-public#632 (comment)

More issues open for this:
lemurheavy/coveralls-public#632
lemurheavy/coveralls-public#487
lemurheavy/coveralls-public#713

@tarasvakulka

Copy link
Copy Markdown

Hi, may be somebody know when this PR will be merged ?

@danielgindi

Copy link
Copy Markdown
ContributorAuthor

@kelset@alloy I know you guys are busy, but this solves a fundamental issue with Android... Can you please take the time to review?

danilobuergerand others added 18 commits June 16, 2021 16:40
Summary:
Adds homebrew on m1 to path before evaluating `command -v brew` to support nvm on m1 via homebrew.
## Changelog
[General] [Changed] - Find node on m1 via homebrew node managers
Pull Request resolved: react#31678
Test Plan:
On M1, use nvm via homebrew. Create a RN project and it'll fail to build iOS app. Apply the patch, and build will succeed.
cc: dulmandakh as discussed in react#31622
Reviewed By: ShikaSD
Differential Revision: D28967386
Pulled By: PeteTheHeat
fbshipit-source-id: 3d4a41dd3cc25fbf77778b16468a236b141d1259
Summary:
Allow you to harvest the `UIAccessibilityContrastHigh` trait from iOS to show accessible colors when high contrast mode is enabled.
```jsx
// usage
PlatformColorIOS({
light: '#eeeeee',
dark: '#333333',
highContrastLight: '#ffffff',
highContrastDark: '#000000',
});
// {
// "dynamic": {
// "light": "#eeeeee",
// "dark": "#333333",
// "highContrastLight": "#ffffff",
// "highContrastDark": "#000000",
// }
// }
```
This is how apple's own dynamic system colors work under the hood (https://developer.apple.com/design/human-interface-guidelines/ios/visual-design/color/#dynamic-system-colors)
---
The react native docs mention that more keys may become available in the future, which this PR is adding:
> In the future, more keys might become available for different user preferences, like high contrast.
https://reactnative.dev/docs/dynamiccolorios
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->
[iOS] [Added] - High contrast dynamic color options for dark and light mode.
Pull Request resolved: react#31651
Test Plan: Added unit tests for `normalizeColor` to pass the high contrast colors downstream to RCTConvert
Reviewed By: lunaleaps
Differential Revision: D28922536
Pulled By: p-sun
fbshipit-source-id: f81417f003c3adefac50e994e62b9be14ffa91a1
…eact#31062)
Summary:
A recent commit react@941bc0e#diff-0eeea47fa4bace26fa6c492a03fa0ea3923a2d8d54b7894f7760cb9131ab65eb on Hermes macOS brings a regression for Mac Catalyst target.
Once hardcoded cli bundle platform `ios` can now be either `ios` or `macos`. However, Mac Catalyst is identified as `macos` rather than `ios`.
This PR should fix it and closereact#31061.
## Changelog
[iOS] [Fixed] - Fix cli bundle platform for Mac Catalyst in `react-native-xcode.sh`
Pull Request resolved: react#31062
Test Plan:
1. Build fails on a new RN 0.64-rc.3 project.
2. Apply the fix.
3. Build passes.
Reviewed By: TheSavior
Differential Revision: D29038793
Pulled By: appden
fbshipit-source-id: 29761f887ec7a9cc26f088953c3888c6d19bed71
Summary:
This PR bumps Fresco to 2.5.0, which is first version on MavenCentral since jCenter announcement.
## Changelog
[Android] [Changed] - Bump Fresco to 2.5.0
Pull Request resolved: react#31699
Test Plan: CI is green
Reviewed By: TheSavior
Differential Revision: D29031847
Pulled By: passy
fbshipit-source-id: 486ffbf5461d07d736c0ebe17c0c7726937db344
Summary:
Pull Request resolved: react#31675
As requested in parent diff, moved the Android dep bumps into a separate diff.
## Changelog
[general][changed] - [Android] Update Flipper to 0.93.0
Reviewed By: mdvacca, ShikaSD
Differential Revision: D28688486
fbshipit-source-id: c3a8e0edeebdabd490b2885497e261f64bdab4bd
Summary: Updating pods attempts to bump fmt to 7.1.3, which causes CircleCI to fail. [Sample failure](https://app.circleci.com/pipelines/github/facebook/react-native/9422/workflows/f4c9076a-9649-490c-b565-555fecc60957/jobs/205827). Let's temporarily lock to an older version until the [fmt fix](fmtlib/fmt@355be4b) gets tagged for release.
Reviewed By: JoshuaGross
Differential Revision: D29147585
fbshipit-source-id: b1aca9618586a9d6e4c6d0c2c37b258745e008ee
Summary:
The native libraries are compiled outside of the usual Android build flow using separate CLI task. Because of that, shared native libraries may not exist when AAR is bundled, resulting in weird sequencing issues.
This change updates gradle dependency graph, executing RN native build before Android part (as it is done in RNTester already).
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D29209249
fbshipit-source-id: 36386c78996b1cd9b1731735e36e571199e9e81b
…lPointerException: bio == null crash (react#31822)
Summary:
Douring our routine crash report check we are occasionally seeing reports of exceptions like this in the wild from our crash stack:
```
java.lang.NullPointerException: bio == null
at com.android.org.conscrypt.NativeCrypto.SSL_pending_written_bytes_in_BIO(NativeCrypto.java)
at com.android.org.conscrypt.NativeSsl$BioWrapper.getPendingWrittenBytes(NativeSsl.java:660)
at com.android.org.conscrypt.ConscryptEngine.pendingOutboundEncryptedBytes(ConscryptEngine.java:566)
at com.android.org.conscrypt.ConscryptEngineSocket.drainOutgoingQueue(ConscryptEngineSocket.java:584)
at com.android.org.conscrypt.ConscryptEngineSocket.close(ConscryptEngineSocket.java:480)
at okhttp3.internal.Util.closeQuietly(Util.kt:501)
at okhttp3.internal.connection.ExchangeFinder.findConnection(ExchangeFile:245)
at okhttp3.internal.connection.ExchangeFinder.findHealthyConnection(ExchangeFile:106)
at okhttp3.internal.connection.ExchangeFinder.find(ExchangeFile:74)
at okhttp3.internal.connection.RealCall.initExchange$okhttp(ExchangeFile:255)
at okhttp3.internal.connection.ConnectInterceptor.intercept(ExchangeFile:32)
...
```
![Screen Shot 2021-07-07 at 1 38 23 PM](https://user-images.githubusercontent.com/8868908/124711795-b5fee980-df28-11eb-98c4-9668661340b6.png)
This appears to only be happening on devices running Android 10 and 11. This happens because there is concurrency issue in Conscrypt where two threads race to close an SSLEngine-based SSLSocket and access to the underlying BIO is unsynchronized.
**The OkHttp team already released a fix for this issue on version 4.9.1** this PR aims to update our OkHttp package to version 4.9.1.
Related discussion:
[https://issuetracker.google.com/issues/177450597](https://issuetracker.google.com/issues/177450597)
[https://publicobject.com/2021/01/30/bio-null/](https://publicobject.com/2021/01/30/bio-null/)
cc dulmandakh fkgozali
## Changelog
[Android] [Changed] - Bumping OkHttp from 4.9.0 to 4.9.1.
Pull Request resolved: react#31822
Test Plan: Manual & Automated from CI
Reviewed By: fkgozali
Differential Revision: D29590198
Pulled By: ShikaSD
fbshipit-source-id: 4228bfd3472114253e13acb436dc1dd9287a148d
Summary:
Fixesreact#31774.
This pull request resolves a problem related to accessing blobs greater than 64 KB on Android. When an object URL for such blob is passed as source of `<Image />` component, the image does not load.
This issue was related to the fact that pipe buffer has a limited capacity of 65536 bytes (https://man7.org/linux/man-pages/man7/pipe.7.html, section "Pipe capacity"). If there is more bytes to be written than free space in the buffer left, the write operation blocks and waits until the content is read from the pipe.
The current implementation of `BlobProvider.openFile` first creates a pipe, then writes the blob data to the pipe and finally returns the read side descriptor of the pipe. For blobs larger than 64 KB, the write operation will block forever, because there are no readers to empty the buffer.
https://github.com/facebook/react-native/blob/41ecccefcf16ac8bcf858dd955af709eb20f7e4a/ReactAndroid/src/main/java/com/facebook/react/modules/blob/BlobProvider.java#L86-L95
This pull request moves the write operation to a separate thread. The read side descriptor is returned immediately so that both writer and reader can work simultaneously. Reading from the pipe empties the buffer and allows the next chunks to be written.
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->
[Android] [Fixed] - Fix support for blobs larger than 64 KB
Pull Request resolved: react#31789
Test Plan:
A new example has been added to RN Tester app to verify if the new implementation properly loads the image of size 455 KB from a blob via object URL passed as image source.
<img src="https://user-images.githubusercontent.com/20516055/123859163-9eba6d80-d924-11eb-8a09-2b1f353bb968.png" alt="Screenshot_1624996413" width="300" />
Reviewed By: ShikaSD
Differential Revision: D29674273
Pulled By: yungsters
fbshipit-source-id: e0ac3ec0a23690b05ab843061803f95f7666c0db
Summary:
# See PR
react#29728
# From PR Author
Using `PlatformColor` with border colors doesn't work currently when switching dark mode as the information is lost when converting to `CGColor`. This change keeps the border colors around as `UIColor` so switching to dark mode works.
```ts
<View
style={{
borderColor: DynamicColorIOS({ dark: "yellow", light: "red" }),
borderWidth: 1,
}}
>
...
</View>
```
This view will start with a red border (assuming light mode when started), but will not change to a yellow border when switching to dark mode. With this PR, the border color will be correctly set to yellow.
## Changelog
[iOS] [Fixed] - Allow PlatformColor to work with border colors
Pull Request resolved: react#29728
Test Plan:
1. Assign a `PlatformColor` or `DynamicColorIOS` to a view border color.
2. Toggle between dark / light mode. See the colors change.
Reviewed By: lunaleaps
Differential Revision: D29268376
Pulled By: p-sun
fbshipit-source-id: 586545b05be0beb0e6e5ace6e3f74b304620ad94
* Bump Flipper to 0.93 (react#31708)
Summary:
Pull Request resolved: react#31708
Changelog:
[general][changed] - [iOS] Update Flipper to 0.93.0
Reviewed By: PeteTheHeat
Differential Revision: D29060305
fbshipit-source-id: 2ff109930437bfc90e8ce441fa681de867206397
* Update Podfile.lock
* bump(hermes): bumped hermes to 0.8.1
While current dependencies included the new version in the range, bumping it make sure that no one use the old one because of a lock file
Co-authored-by: Michel Weststrate <mweststrate@fb.com>
@danielgindi
danielgindiforce-pushed the feature/android_clip_touches branch from e64cc28 to 4f72040CompareJuly 26, 2021 07:28
@pull-bot

pull-bot commented Jul 26, 2021

Copy link
Copy Markdown
Fails
🚫

❗ Base Branch - The base branch for this PR is something other than master. Are you sure you want to target something other than the master branch?

Warnings
⚠️🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against c6c3aa2

@danielgindi
danielgindiforce-pushed the feature/android_clip_touches branch from 4f72040 to c6c3aa2CompareJuly 26, 2021 10:47
@danielgindi

Copy link
Copy Markdown
ContributorAuthor

Finally, the PR by @hsource was merged. This older one is now obsolete.
My merged branch will still be available for use until this will be released, then I'll delete it.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BugCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.Platform: AndroidAndroid applications.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

20 participants

@danielgindi@nick-michael@ltcaosj@kdenz@fabOnReact@tarasvakulka@kelset@EzeMortal@TheMrZZ@owpac@analysis-bot@aanation@1awaleed@peetklecha@daxfrost@Rodrigo816@Ragnar-H@qalqi@shevchenkona19@tolgacesur