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

[SuperEditor][Android][iOS] - Add tooling to more easily create keyboard toolbar experiences (Resolves #2209) #2216

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

Conversation

angelosilvestre
Copy link
Collaborator

[SuperEditor][Android][iOS] - Add tooling to more easily create keyboard toolbar experiences. Resolves #2209

This PR introduces the KeyboardPanelScaffold widget to make it easier to implement a panel behind keyboard widget. This is common for chat apps, where the user can switch between the regular software keyboard panel and an emoji panel.

This widget does not enforce any particular styling on the panel. I named it "keyboard panel" because the lack of a better name, maybe we could find a better name for it.

I modified the existing PanelBehindKeyboardDemo demo to use this new widget and also modified the demo to add content to the keyboard panel. The demo mentions it's only for Android, but I didn't find any issues on iOS.

One issue that I found is that our SoftwareKeyboardController.close closes the IME connection instead of just closing the keyboard. This might cause problems. We should probably be invoking TextInput.hide instead.

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-08-04.at.23.00.54.mp4
android_panel.webm

/// visible, or above the software keyboard, when visible. If neither the keyboard panel nor
/// the software keyboard are visible, the widget is positioned at the bottom of the screen.
///
/// The widget returned by [contentBuilder] is positioned at the top, using all the available
Copy link
Contributor

Choose a reason for hiding this comment

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

Please describe contentBuilder a bit more. It's a bit confusing to read "positioned at the top" right after reading that "[aboveKeyboardBuilder] is positioned above the keyboard panel"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated explaining the it's positioned above the above-keyboard panel. Which kind of other information do you think I should add here?

///
/// Use the [controller] to show/hide the keyboard panel and software keyboard.
///
/// It is required that [Scaffold.resizeToAvoidBottomInset] is set to `false`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please elaborate on this requirement. Which Scaffold? And why is it required? What happens if it's true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.

@matthew-carroll
Copy link
Contributor

Some thoughts from trying to use this in a chat editor use-case:

  • We should have an auto-display property such that, when true, the toolbar content is displayed whenever the editor has a selection.
  • The content currently flows behind the toolbar, but this hides some of the content. This is especially a problem when the editor is a small, intrinsic height editor at the bottom of the screen. Essentially the whole editor disappears behind the toolbar.
    • We should have a widget property option to decide whether to expand the content behind the toolbar area or not.
    • This will be a little tricky to implement because the toolbar is in the overlay, and we need to add space below the content that's equal to the intrinsic height of the toolbar. Maybe a two frame sequence is needed - one frame that displayed the toolbar offscreen and measures it, and then on the second frame, once its measured, both the toolbar and the content are displayed (to prevent a flash where the editor is in the wrong place).
  • Possible bug: In the app where I'm trying this out, sometimes I hot reload and the toolbar disappears. Not sure if that's a problem in the scaffold or this app.

@angelosilvestre
Copy link
Collaborator Author

The content currently flows behind the toolbar, but this hides some of the content. This is especially a problem when the editor is a small, intrinsic height editor at the bottom of the screen. Essentially the whole editor disappears behind the toolbar.

@matthew-carroll I added a widget to added a widget to address this issue. Updated the chat demo to use it.

keyboard_toolbar.webm

@matthew-carroll
Copy link
Contributor

@angelosilvestre I saw your commit saying that you added an option to auto show the toolbar above the keyboard, but I don't actually see any public options added in that commit.

How does one say whether they want the toolbar to be displayed automatically or not?

@matthew-carroll
Copy link
Contributor

It looks like a good number of tests are failing.

@matthew-carroll
Copy link
Contributor

matthew-carroll commented Sep 25, 2024

I added a KeyboardScaffoldSafeArea in a location in the widget tree where I think it should make sense, but it didn't have any impact on the content location. The content still moves down behind the keyboard panel.

Is there anything I should know about how to use this?

Looks like I had the safe area in the wrong part of the widget tree.

However, now that I have it in the correct area, some part of these widgets is now pushing my content up even when the keyboard is open. So now I've got the keyboard at the bottom of the screen, then a bunch of white space, then my content up near the very top of the screen.

When I press a button on the toolbar to open the keyboard panel, then the content comes back down to where it's supposed to be. However, when I open the keyboard panel, the toolbar seems to disappear. There's space for the toolbar, but the toolbar is gone.

@matthew-carroll
Copy link
Contributor

@angelosilvestre can you take a look at the modifications I've made? Before ClickUp can use this, we'll need to merge it in and cut a new release. So it would be great if we could finalize this pretty quickly. Let me know if you see anything concerning in here.

Copy link
Collaborator Author

@angelosilvestre angelosilvestre left a comment

Choose a reason for hiding this comment

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

Comment on lines 189 to 190
// Currently, closing the software keyboard causes the IME connection to close.
clearSelectionWhenImeConnectionCloses: false,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this still true after your latest changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you see any area where it changed? I didn't alter any existing tests related to that, and I don't recall altering any related code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to merge this. Please let me know if you have any evidence that this behavior changed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought we have talked about that, but I might be mistaken.

@matthew-carroll
Copy link
Contributor

@angelosilvestre can you do a final review of this? I think we've got it working in ClickUp. I'm sure it'll evolve, but let's get this version merged and released.

Copy link
Collaborator Author

@angelosilvestre angelosilvestre left a comment

Choose a reason for hiding this comment

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

LGTM with a few comments.

Comment on lines 381 to 383
// Updates our local cache of the current bottom window insets, which we assume reflects
// the current software keyboard height.
void _updateKeyboardHeightForCurrentViewInsets() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did you intend to leave this as a regular comment instead of a dart doc?

},
);
},
child: Padding(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can we remove this padding?

Comment on lines 454 to 460
double get _keyboardPanelHeight {
return _wantsToShowKeyboardPanel //
? _keyboardHeight.value < 100 //
? widget.fallbackPanelHeight
: _keyboardHeight.value
: 0.0;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's probably a good idea to add a comment explaining what the keyboard height being smaller than 100 means.

Comment on lines 470 to 474
final double fakeKeyboardHeight = _wantsToShowKeyboardPanel //
? _keyboardHeight.value < 100 //
? widget.fallbackPanelHeight
: 0.0
: 0.0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's probably a good idea to add a comment explaining what the keyboard height being smaller than 100 means.

)
: null,
body: superEditor,
resizeToAvoidBottomInset: false,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be a good idea to leave a comment here explaining why we are always setting resizeToAvoidBottomInset to false.

});

group('Android tablets >', () {
testWidgetsOnIPad('shows panel when keyboard is docked', (tester) async {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should this be testWidgetsOnAndroidTablet ?

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.

[SuperEditor][Android][iOS] - Add tooling to more easily create keyboard toolbar experiences
2 participants