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

Avoid scrolling to cursor when tap on a checkbox #400

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kfdykme
Copy link
Contributor

@kfdykme kfdykme commented Aug 11, 2024

Hi, I don't know if this is a bug or a feature. But every time I open FleatherEditor, scroll to the bottom, and just want to change the checkbox, it scrolls to the top. I feel bad, so is it possible to make it optional?

Please give me some advice? I'll add a test if I can.

Fleather.-.rich-text.editor.for.Flutter.-.1.-.Microsoft.Edge.2024-08-12.00-42-36.mp4

@amantoux
Copy link
Member

@kfdykme do you know the root cause of the scroll jump?
Your solution seems to workaround a bug

@kfdykme
Copy link
Contributor Author

kfdykme commented Aug 12, 2024

@amantoux the root cause is that whenever the editor content changes, the editor scrolls to the cursor position. I think this is ok. But sometimes I don't need this functionality, I just want to change the checkbox state without doing any editing.

steps:

  1. tap checkbox
  2. change checkbox value
  3. format text value
    ---------------- notifyListener -------------------
  4. onEditingValueChange
  5. _showCaretOnScreen was called
  6. jump to caret position

4,5,6 are one module, 1,2,3 are another module, they are running correctly according to their roles. When they run together, it feels weird.

Another solution is to select the row of the checkbox when the checkbox is clicked?

@amantoux
Copy link
Member

@kfdykme @Amir-P
Some thoughts:

  • Do we want to jump to cursor whenever formatText is invoked? Do you have use cases in mind where it would be necessary?
  • When collaborative edition is active, we might want to have the possibility to avoid jumping to current cursor?

@amantoux amantoux changed the title Featire or Bugfix? enable skip scroll while tap on a checkbox Feature or Bugfix? enable skip scroll while tap on a checkbox Aug 12, 2024
@amantoux amantoux changed the title Feature or Bugfix? enable skip scroll while tap on a checkbox Avoid scrolling to cursor when tap on a checkbox Aug 12, 2024
@Amir-P
Copy link
Member

Amir-P commented Aug 25, 2024

@kfdykme @Amir-P Some thoughts:

  • Do we want to jump to cursor whenever formatText is invoked? Do you have use cases in mind where it would be necessary?
  • When collaborative edition is active, we might want to have the possibility to avoid jumping to current cursor?
  • I believe it has some use cases. Imagine user has selected a part of text then scrolls and then taps on a format. Or user put cursor in a part of text, scroll, and then taps on code block. I think in those cases the expected behavior would be to jump to the selected area.
  • Yes. It's like the other bug that we have already fixed (Do not open the keyboard when a checkbox is toggled #301).

@kfdykme
Copy link
Contributor Author

kfdykme commented Aug 26, 2024

@kfdykme @Amir-P Some thoughts:

  • Do we want to jump to cursor whenever formatText is invoked? Do you have use cases in mind where it would be necessary?
  • When collaborative edition is active, we might want to have the possibility to avoid jumping to current cursor?
  • I believe it has some use cases. Imagine user has selected a part of text then scrolls and then taps on a format. Or user put cursor in a part of text, scroll, and then taps on code block. I think in those cases the expected behavior would be to jump to the selected area.
  • Yes. It's like the other bug that we have already fixed (Do not open the keyboard when a checkbox is toggled #301).
image It won't request the keyboard, but it will scroll to the cursor using _showCaretOnScreen : image

When you tap on a checkbox, the cursor won't change because the selection doesn't change by simply tapping the checkbox. As a result, it may scroll to a strange position.

Here are two possible solutions that might be useful:

Provide an option to skip scrolling when tapping on a checkbox. This allows users to choose whether they want the scrolling behavior or not.

Change the selection position before scrolling to ensure that it aligns with the checkbox value change. This ensures that the cursor is in the correct position before any scrolling occurs.

@amantoux
Copy link
Member

@kfdykme I agree with the two possible solutions.
My previous comment was to ensure that adding the optional arg you've added was in line with future evolutions

The conclusion is that I think it is

@Amir-P
Copy link
Member

Amir-P commented Aug 26, 2024

It won't request the keyboard, but it will scroll to the cursor using _showCaretOnScreen :

@kfdykme Sorry for not being clear. I meant that it's "like" the other issue where it also affected the collaborative editing experience and this issue reminded me of that. Not that they are the same issue.

Although, I'm not in favor of having this workaround in the Fleather. The checkbox right now is the only interactive widget in Fleather, but it might change in future. Also it does not fix the issue with remote changes. @amantoux

@amantoux
Copy link
Member

@kfdykme while we decide on the best approach
Another solution could be to unfocus the document when the checkbox is ticked
WDYT?

@amantoux
Copy link
Member

It won't request the keyboard, but it will scroll to the cursor using _showCaretOnScreen :

@kfdykme Sorry for not being clear. I meant that it's "like" the other issue where it also affected the collaborative editing experience and this issue reminded me of that. Not that they are the same issue.

Although, I'm not in favor of having this workaround in the Fleather. The checkbox right now is the only interactive widget in Fleather, but it might change in future. Also it does not fix the issue with remote changes. @amantoux

@Amir-P your previous statement acknowledges the fact that there is a general need to support changes without affecting the scroll position so I'm a little confused

Can you be more specific?

@Amir-P
Copy link
Member

Amir-P commented Aug 26, 2024

Can you be more specific?

@amantoux Let me recap my points:

  1. This is an issue that we definitely need to fix especially if we consider collaborative editing.
  2. We can't just remove the current scroll to cursor behavior to fix this issue since we have some cases where it is useful. Like when user creates a quote block or code block. Or user has selected a text segment and clicks on an inline format.
  3. I'm not in favor of "this" fix since it's not scalable, and does not fix issue with collaborative editing where remote changes can happen.

@kfdykme
Copy link
Contributor Author

kfdykme commented Sep 3, 2024

@kfdykme while we decide on the best approach Another solution could be to unfocus the document when the checkbox is ticked WDYT?

If a user is editing, do they also need to cancel the focus?

@kfdykme
Copy link
Contributor Author

kfdykme commented Sep 3, 2024

It won't request the keyboard, but it will scroll to the cursor using _showCaretOnScreen :

@kfdykme Sorry for not being clear. I meant that it's "like" the other issue where it also affected the collaborative editing experience and this issue reminded me of that. Not that they are the same issue.

Although, I'm not in favor of having this workaround in the Fleather. The checkbox right now is the only interactive widget in Fleather, but it might change in future. Also it does not fix the issue with remote changes. @amantoux

Can I assume that this issue concerns the checkbox or other future prepended widgets, but should remain self-consistent inside the widget, rather than being a global option controlled by the controller. Therefore, I can fix it directly at the checkbox level, rather than modifying the controller.

@amantoux
Copy link
Member

amantoux commented Sep 7, 2024

@kfdykme while we decide on the best approach Another solution could be to unfocus the document when the checkbox is ticked WDYT?

If a user is editing, do they also need to cancel the focus?

What I mean is that when the user clicks on the checkbox, maybe we should remove focus from editor prior to updating parchment via the controller to avoid the scroll jump-to, I didn't check if this was feasible but it could be a desired behavior.
WDYT? @kfdykme

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.

3 participants