flutter-quill icon indicating copy to clipboard operation
flutter-quill copied to clipboard

Fix caret not auto scroll to visible when page has multiple editors

Open theachoem opened this issue 7 months ago • 5 comments

Description

For this fix, I copied all code based on flutter editable_text.dart to have the same behavior. I have pushed multiple commits to track the changes:

  • The first commit is just copied original code from Flutter (no change): editable_text.dart#L4228
  • The second commit is refactoring to fit flutter_quill

Related Issues

  • https://github.com/singerdmx/flutter-quill/issues/2569

Fixed Demo

https://github.com/user-attachments/assets/ae027618-21c6-499f-a3e8-4bc338f79a18

Type of Change

  • [ ] ✨ Feature: New functionality without breaking existing features.
  • [x] 🛠️ Bug fix: Resolves an issue without altering current behavior.
  • [ ] 🧹 Refactor: Code reorganization, no behavior change.
  • [ ] ❌ Breaking: Alters existing functionality and requires updates.
  • [ ] 🧪 Tests: New or modified tests
  • [ ] 📝 Documentation: Updates or additions to documentation.
  • [ ] 🗑️ Chore: Routine tasks, or maintenance.
  • [ ] ✅ Build configuration change: Build/configuration changes.

theachoem avatar May 17 '25 18:05 theachoem

Mostly works, but ran into a bit of an issue, it does not auto-scroll on the first request focus until I start typing:

https://github.com/user-attachments/assets/63dd56bc-69f4-4443-8928-dc4eb1687293

theachoem avatar May 17 '25 18:05 theachoem

Mostly works, but ran into a bit of an issue, it does not auto-scroll on the first request focus until I start typing: https://github.com/user-attachments/assets/63dd56bc-69f4-4443-8928-dc4eb1687293

Handle didChangeMetrics fix the issue:

https://github.com/user-attachments/assets/1bfd2b22-85a2-4bfc-8e8a-c72bce463440

Reference:

theachoem avatar May 17 '25 19:05 theachoem

@EchoEllet I have released this to my production app for a few days now. If you have any concerns about it being merged, let me know. I will refactor accordingly.

theachoem avatar May 20 '25 14:05 theachoem

@EchoEllet I understand. I am confident about it, and I think if other contributors do it, it will likely output the same result. Let me know what I can do.

theachoem avatar May 20 '25 17:05 theachoem

Seem like quality check no longer work because flutter just release new stable version. Let me fix that.

theachoem avatar May 20 '25 18:05 theachoem

@EchoEllet shall we merge this?

singerdmx avatar Jun 15 '25 18:06 singerdmx

I'm not able to review it in a reasonable time frame, sorry but I can't confirm my review. Please make the call.

EchoEllet avatar Jun 15 '25 19:06 EchoEllet

How can I implement this? I still have this problem. Thanks a lot!

anneinge95 avatar Jun 18 '25 20:06 anneinge95

You can point to the master branch directly.

theachoem avatar Jun 19 '25 06:06 theachoem

It is working, thanks a lot for the work! However, the cursor scrolls behind/under the custom toolbar. Auto-scroll only accounts for the keyboard height, not additional UI elements above it.

anneinge95 avatar Jun 20 '25 09:06 anneinge95

@anneinge95 works fine for me, make sure you put the toolbar in the bottom navigation.

theachoem avatar Jun 29 '25 07:06 theachoem

I reverted this PR because it includes unrelated changes beyond the intended fix. To maintain stability, we avoid introducing breaking changes unless absolutely necessary. Please consider submitting a focused PR with only the relevant fix.

EchoEllet avatar Jul 22 '25 01:07 EchoEllet

I reverted this PR because it includes unrelated changes beyond the intended fix. To maintain stability, we avoid introducing breaking changes unless absolutely necessary. Please consider submitting a focused PR with only the relevant fix.

Thanks for the update. However, all the changes in that PR are necessary and part of the intended fix. The breaking changes were required to access the padding.bottom, or padding.copyWith instead of just horizonal or vertical value.

Please consider reverting it back, let me know what I can help.

theachoem avatar Jul 22 '25 01:07 theachoem

let me know what I can help.

This isn't an issue with your PR; it's a project maintenance issue.

However, all the changes in that PR are necessary and part of the intended fix

The lint fixes are not part of the PR fix, and it's a CI issue.

For this fix, I copied all code based on flutter editable_text.dart to have the same behavior.

Generally, I prefer to avoid the approach of copying complex code and modifying it. It was done previously with the magnifier feature, but had to revert it due to significant regressions.

I'm no longer a project maintainer, but It looks like PRs are being merged without any review, consideration, testing, or documentation, so every consumer is a testing site. I have seen this pattern before when I was a maintainer, and I still see it today.

Unfortunately, it looks like there are no longer active project maintainers, so for new projects I highly recommend alternatives such as super_editor or appflowy_editor.

EchoEllet avatar Jul 22 '25 01:07 EchoEllet

The lint fixes are not part of the PR fix, and it's a CI issue.

I agree, but if I don’t fix the lint, the CI fails and the PR can’t be merged.

Generally, I prefer to avoid the approach of copying complex code and modifying it. It was done previously with the magnifier feature, but https://github.com/singerdmx/flutter-quill/pull/2413 due to significant regressions.

I get that. But Flutter Quill already copies a lot from Flutter’s text field, and this helps keep behavior consistent. In this case, copying from editable_text.dart was needed for things to work properly. It also keeps up to date by comparing with latest text_field.

Unfortunately, it looks like there are no longer active project maintainers, so for new projects I highly recommend alternatives such as super_editor or appflowy_editor.

That might be true, but the project still works well for my app, so I don’t see a need to switch right now.

theachoem avatar Jul 22 '25 02:07 theachoem

Merging it might work for your app, but without proper testing, I can't risk merging it for everyone. I'm not the original maintainer and trying to leave—please consider forking if you need it.

However, I appreciate your efforts on this PR, but it's a project issue rather than an issue with your PR.

But Flutter Quill already copies a lot from Flutter’s text field

We should improve the situation instead of blindly copy-pasting code we don't understand (Flutter internal) and shipping it to the users of this package.

I agree, but if I don’t fix the lint, the CI fails and the PR can’t be merged.

This can be fixed without introducing the change as part of this PR. This makes the revert (if we decided to make it) harder with many conflicts.

That might be true, but the project still works well for my app,

You could use a dependency override with your fork, and pull changes from upstream repo to upgrade.

To explain the situation (off-topic)

I wanted to quit Flutter Quill 1 or 2 years ago, but all the previous maintainers have abandoned the project, so I'm the only one who is working with the project (see commit history).

I don't think we're as strict as we used to be. PRs are being merged without enough consideration, and when regressions happen, I'm often the only one left to fix them and clean up the mess. Maintaining a codebase like Flutter Quill is time-consuming for anyone who has worked with its internals.

EchoEllet avatar Jul 22 '25 03:07 EchoEllet

any idea how to fix this ?

Aruljebaraj avatar Aug 01 '25 10:08 Aruljebaraj

Is there a new solution?

anneinge95 avatar Aug 04 '25 10:08 anneinge95