suragch / mongol

Mongolian vertical script widgets for Flutter apps
https://pub.dev/packages/mongol
BSD 3-Clause "New" or "Revised" License
70 stars 15 forks source link

DefaultMongolTextEditingShortcuts Widget Cannot be Positioned in MongolEditableText Widget #33

Closed Satsrag closed 1 year ago

Satsrag commented 1 year ago

image

DefaultMongolTextEditingShortcuts widget intercepts keyboard key events (such as the key up/down) and forwards to MongolEditableText's Actions Widget to handle key events. It switches the up/down key and the left/right key.

However Focus widget cannot receive all key events when the Focus widget is positioned at the position in the figure above. It just only can handle the MongolEditableText's Actions widget unhandled key events.

Flutter SDK puts the DefaultTextEditingShortcuts widget in the WidgetsApp widget, so the Focus widget can receive all key events.

There are some solutions:

I prefer the last solution. In this way, developers using this library don't have to care about more things. But it may be a long job. So do we use the first solution first and then work on the last one? Or any other ideas?

suragch commented 1 year ago

Based on your description, I think I like the third solution better, too. Developers shouldn't need to understand focus and shortcut handling in order to use the library. Everything should work and feel like standard Flutter widgets.

Before we start making changes, though, can you help me understand the problem better? What currently doesn't work. Is it demonstratable in the demo app? Is it that the arrow keys don't work? If the problem can't be demonstrated in the demo app, would you mind making a minimal Flutter project that shows the problem?

Satsrag commented 1 year ago
import 'package:flutter/material.dart';
import 'package:flutter/services.dart';
import 'package:mongol/mongol.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  static const String _title = 'Focus Sample';

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: _title,
      home: Scaffold(
        appBar: AppBar(title: const Text(_title)),
        body: Center(
          child: SizedBox(
            width: 300,
            height: 300,
            child: Focus(
              onKey: (node, event) {
                if (event.logicalKey == LogicalKeyboardKey.backspace ||
                    event.logicalKey == LogicalKeyboardKey.space) {
                  print("MyApp -> intercept key: ${event.logicalKey.keyLabel}");
                  return KeyEventResult.handled;
                } else {
                  print("MyApp -> ignore key: ${event.logicalKey.keyLabel}");
                  return KeyEventResult.ignored;
                }
              },
              child: const MongolTextField(),
            ),
          ),
        ),
      ),
    );
  }
}

Copy this demo code to a Flutter project and run it on the Web. Press the space or the backspace key. These two keys should be intercepted by the Focus widget and should not be input in the MongolTextField.

Replace MongolTextField with TextField in the demo code, it will work well (Pressing space or backspace will be intercepted by the Focus widget).

This demo code should be run on the web. Other platforms have no problem. Because the DefaultMongolTextEditingShortcuts._webDisablingTextShortcuts intercept these keys on web and don't intercept on other platforms. I think we will open a new issue and talk about whether these keys should be intercepted.

Flutter official link about: Action and Shortcuts Focus

suragch commented 1 year ago

So you are saying this only affects the web currently, right? And it only affects the web when you are using a Focus widget to capture key events? What is the use case for this? I mean, what made you discover this bug? Sorry for all of my questions. I'm still trying to understand this issue.

Feel free to open another issue if you want or we can keep discussing this here.

In general, my thought is, if we can find a solution that doesn't break any existing user code or change the public API, then let's do whatever works, especially if it doesn't require the users to learn more things.

You're the one who brought MongolTextField back to life. If it weren't for you, we wouldn't have it at all. So I trust your judgement.

Satsrag commented 1 year ago

I'm sorry about the above demo code. It does not reflect this issue well. It only reflects my use case.

In my use case, This only affects the web. However, in another use case, it will affect the other platforms. For example, if someone wants to intercept LogicalKeyboardKey.arrowDown, just replace the keys that need to be intercepted with LogicalKeyboardKey.arrowDown in the above demo code, this issue occurs on all platforms. The corrected demo code:

import 'package:flutter/material.dart';
import 'package:flutter/services.dart';
import 'package:mongol/mongol.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  static const String _title = 'Focus Sample';

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: _title,
      home: Scaffold(
        appBar: AppBar(title: const Text(_title)),
        body: Center(
          child: SizedBox(
            width: 300,
            height: 300,
            child: Focus(
              onKey: (node, event) {
                if (event.logicalKey == LogicalKeyboardKey.arrowDown ||
                    event.logicalKey == LogicalKeyboardKey.arrowUp) {
                  print("MyApp -> intercept key: ${event.logicalKey.keyLabel}");
                  return KeyEventResult.handled;
                } else {
                  print("MyApp -> ignore key: ${event.logicalKey.keyLabel}");
                  return KeyEventResult.ignored;
                }
              },
              child: const MongolTextField(),
            ),
          ),
        ),
      ),
    );
  }
}

I'm coding an embedded IME/Keyboard. This is my use case. I want to open-source it in the future. However, right now it is only the demo code and needs a big refactor and other things. I have removed the thesaurus DB from it. This DB is not mine and I will make it or get his permission before open-source this library.

Satsrag commented 1 year ago

I have pulled request #35. It is the second solution. It removes DefaultMongolTextEditingShortcuts from MongolEditableTextState. Developers using this library need to insert DefaultMongolTextEditingShortcuts to WidgetsApp's builder. So DefaultMongolTextEditingShortcuts is a sub widget of DefaultTextEditingShortcuts and a parent widget of the all app widgets.

suragch commented 1 year ago

So as I understand it, any time a user wants to handle logical keyboard keys, they will need to add the DefaultMongolTextEditingShortcuts (or MongolTextEditingShortcuts as I am recommending it be renamed) at the WidgetsApp level, right?

That means this will affect desktop and web but not mobile, correct?

Satsrag commented 1 year ago

The MongolTextEditingShortcuts is just switching up/down and left/right keys. If developers using this library will not add it to WidgetsApp's builder, the MongolEditableText handle shortcuts the same as EditableText. if the developers declare it will switch up/down and left/right keys.

Normally, this will affect the desktop and web. However, if the mobile is plugged into the hard keyboard, this will affect the mobile.

Please, declare the usage of MongolTextEditingShortcuts to README. I'm not good at English. Thank you!

suragch commented 1 year ago

@Satsrag Thank you for your explanations and pull request. I think I understand the problem better now. I've merged your pull request and have also updated some of the documentation. Can you check my changes? If they look good to you, I'll publish the next version.

Satsrag commented 1 year ago

@suragch I have checked it. Everything is ok. Thank you!

suragch commented 1 year ago

@Satsrag Thanks! It's on pub now.