Skip to content

feat: OverKeyboardView with custom ShadowNode #863

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

Merged
merged 18 commits into from
Mar 25, 2025

Conversation

kirillzyusko
Copy link
Owner

@kirillzyusko kirillzyusko commented Mar 16, 2025

📜 Description

Stretch OverKeyboardView to full screen height on fabric/android.

💡 Motivation and Context

To make OverKeyboardView to stretch to full screen we have to override layout on ShadowNodes side. For paper architecture we already do that in corresponding ShadowNode kotlin class. Since new architecture is C++ based - we have to make corresponding changes in C++ code.

Unfortunately there is no easy way to add custom properties/code in existing/auto-generated shadow-nodes. To make it working we have to genereate them ourself and change the code accordingly. In this PR I did that. I copied autogenerated code, formatted it according to cpplint rules and did a proper linking. After that I wrote additional code that uses values passed from kotlin in c++ and properly resized the inner view.

Last, but not least - on iOS we manually specify the size of inner child, on Android - we are laid out by ShadowNodes. To keep the same behavior on Android we need to stretch to full screen width top: 0, right: 0, bottom: 0, left: 0, while on iOS we just have to specify exact dimensions.

Also on Android/Fabric I discovered, that if view keeps mounted, then the view intercepts touches (even if it's not laid out properly). To fix that and match RN behavior I added visible && children condition - in this case we don't mount children and thus they are not clickable 🙃

Closes #862

📢 Changelog

Example

  • added black color for "KYC" button (toolbar);

JS

  • removed architecture.ts file;
  • generate interfaceOnly from codegen;
  • don't mount children until visible=true (to match Modal behavior);
  • added react-native.config.js;

C++

  • created common folder that contains all ShadowNodes;

iOS

  • added common dependency in Podfile for new architecture;

Android

  • added jni folder;
  • added CMakeList.txt;
  • added custom detekt config;
  • pass StateWrapper to OverKeyboardView;
  • added stretchTo function to OverKeyboardView;

🤔 How Has This Been Tested?

Tested locally on Medium Phone API 35 (paper and fabric) and on CI via e2e tests.

📸 Screenshots (if appropriate):

Before After
image image

📝 Checklist

  • CI successfully passed
  • I added new mocks and corresponding unit-tests if library API was changed

@kirillzyusko kirillzyusko self-assigned this Mar 16, 2025
@kirillzyusko kirillzyusko added 🏭 fabric Changes specific to new (fabric/jsi) architecture OverKeyboardView Anything related to OverKeyboardView 🐛 bug Something isn't working labels Mar 16, 2025
Copy link
Contributor

github-actions bot commented Mar 16, 2025

📊 Package size report

Current size Target Size Difference
173342 bytes 170428 bytes 2914 bytes 📈

@kirillzyusko kirillzyusko changed the title feat: OverKeyboardView custom ShadowNode feat: OverKeyboardView with custom ShadowNode Mar 16, 2025
@kirillzyusko kirillzyusko force-pushed the feat/over-keyboard-view-custom-shadow-node branch from 3434212 to 7082b59 Compare March 19, 2025 10:35
@kirillzyusko kirillzyusko marked this pull request as ready for review March 25, 2025 10:17
@kirillzyusko kirillzyusko merged commit 03937a7 into main Mar 25, 2025
26 checks passed
@kirillzyusko kirillzyusko deleted the feat/over-keyboard-view-custom-shadow-node branch March 25, 2025 10:18
@Mako-L
Copy link
Contributor

Mako-L commented Mar 25, 2025

This looks amazing! @kirillzyusko When we will have a new version of the package including this?

@kirillzyusko
Copy link
Owner Author

@Mako-L I will publish it under 1.17.0, and it may take several weeks (I want to merge some other PRs and then release 1.17.0).

In the meantime if you want you can use dependency from a main branch (directly from github).

@Mako-L
Copy link
Contributor

Mako-L commented Mar 26, 2025

@kirillzyusko I just tried the package from main branch and now there is other issue the 'TouchableWithoutFeedback' from example in documentation press doesn't trigger on all the screen it only runs the tap function only on the bottom like around 200px height or something like that. I will open a new ticket and update my repo to reproduce the bug. Thanks again.

Edit:
On demo code apparently it works when i put it in new repo so I must be doing something wrong in my main code. Sorry for the misunderstanding.

@kirillzyusko
Copy link
Owner Author

@Mako-L yeah, I tested example app a lot to be sure that everything works identical to how it worked on paper

But let me know if you discover any issues! I'll be happy to fix them before a new release ☺️

@Mako-L
Copy link
Contributor

Mako-L commented Mar 26, 2025

@kirillzyusko The problem appears when using KeyboardStickyView with OverKeyboardView. Opened another report: Issue

@kirillzyusko kirillzyusko mentioned this pull request Apr 11, 2025
2 tasks
kirillzyusko added a commit that referenced this pull request Apr 11, 2025
## 📜 Description

Wrapped touch events in `try`/`catch`.

## 💡 Motivation and Context

The problem stems from the fact, that we are trying to cast
`ViewRootImpl` to `ViewGroup` (viewrootimpl cannot be cast to
android.view.viewgroup). Basically it happens when touch gets propagated
until `RootView` and wasn't handled earlier. Such thing can happen when:
- view is not fully laid out but it already handles a touch;
- view is not stretched to full screen and you touch area that doesn't
belong to `OverKeyboardView` (fixed in
#863)

I was aware about that issue (especially on Fabric) and decided not to
fix it - the main motivation was to stretch view to full screen on
Fabric. However now, when view gets stretched we still have a small race
condition when users click fast (i. e. view is not laid out but user
already pass a touch).

I don't know a proper fix for this problem, and for me it looks like it
can be OS scheduling/management which can not be fixed because I don't
have an access to it 😅

So in this PR I simply wrap the touch handling in `try`/`catch`. If
touch can not be handled then it's better to silently fail instead of
crashing the app.

Closes
#884

## 📢 Changelog

<!-- High level overview of important changes -->
<!-- For example: fixed status bar manipulation; added new types
declarations; -->
<!-- If your changes don't affect one of platform/language below - then
remove this platform/language -->

### Android

- wrapped `handleTouchEvent` calls inside `try`/`catch` block to prevent
a crash;

## 🤔 How Has This Been Tested?

Tested manually on Medium Phone API 35.

## 📸 Screenshots (if appropriate):


https://github.com/user-attachments/assets/9ffa684d-ddb7-46e5-b143-d526ab680d28

## 📝 Checklist

- [x] CI successfully passed
- [x] I added new mocks and corresponding unit-tests if library API was
changed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🏭 fabric Changes specific to new (fabric/jsi) architecture OverKeyboardView Anything related to OverKeyboardView
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[android] OverKeyboardView after upgrading to expo 52 doesnt use all screen and when press outside crashes the app
2 participants