Fix selection in readOnly mode, Add magnifier via RawMagnifier widget #2529
Conversation
|
From what I see, this restores the magnifier behavior, right? We've had issues with this implementation before, and it led to some problems during rollbacks. For the reasons mentioned above, it's best to test this PR until we know it won't cause us problems, like it did last time. In any case, thank you for taking the time and effort to make these changes. |
|
This does restore magnifier behavior. I did not create the previous magnifier implementation, but I did fix some of it's bugs that had been merged. However, this is a completely different implementation that is using Flutter's RawMagnifier widget in the Widget tree (see text_selection.dart) instead of changing events / config / etc throughout the editor. The other dart changes in this PR are just passing a ValueNotifier around, and that ValueNotifier is only used in text_selection.dart. (This PR also has some selection fixes when in read only mode) I agree needs to be tested. I have an internal team of 8, 1.2 million app users creating just over 10K notes a day. They have been running the 10.8.5 version that had my previous magnifier fixes. Prior to the fixes, we received several support tickets about the magnifier bugs, since the fixes no reports. After we finish internal testing, we will do a rollout to our users and the feature will be used there. We have finished updating to flutter_quill 11.x and require the selection and magnification changes. |
EchoEllet
changed the title
…llMangifierBuilder, also added a defaultQuillMagnifierBuilder function, when not specified the magnifier will not be used.
|
|
This reverts commit 45844a3. Reasons to revert: * For full text selectable readonly editor contents, `SelectableText` only works on `TextSpan` children. `WidgetSpan` will make a gap for text selection and implementing a `SelectableWidgetSpan` combines both kinds of span is not easy to do: assert failure everywhere. * `SelectableArea` handles `onTap` event where we only want the gesture passed to children so that users could activate reply state by a single tap, to fix the issue we need a custom ignore pointer widget. * Rendered style not consists comparing the editor when editing and viewing page contents because the later one uses custom html parser while the former one uses editor which renders custom bbcode tags, they are not the same, both in history and in the future, as long as the process differs. And the html parser is quite limited and not stable, endless html layout rules are not clearly implemented, especially in line break. Frequently used layout are supported but not always be, massive special logic in the custom html parser makes it hard to maintain, let alone the selectable text feature we want to land in a future version. I think it's time to sunset the current html parser, or at least render into quill_delta so that we can have consistent contents appearance when editing and viewing contents. Next step: * Other implementation may not work, like use `Wrap` to simulate the original rich text features in flutter SDK, I don't think the result could be better than the one in SDK. * We need selectable text and consistent appearance when editing and viewing contents so render it in the bbcode editor is the best choice. Waiting for upstream fix selection in readOnly mode here: singerdmx/flutter-quill#2529 After that, parse supported tags here: https://github.com/realth000/tsdm_client/blob/81395ca8f5f5ee9ebe63cff1d223f585a895bf9a/lib/utils/html/html_muncher.dart#L335 into custom bbcode tags to let the bbcode editor render it as common quill_delta tags, hard but worth it.
|
Sorry for the delay, is the fix of selection in readOnly mode related to the PR #2488? |
| @@ -853,8 +853,9 @@ class QuillRawEditorState extends EditorState | |||
| }); | |||
| } | |||
|
|
|||
| controller.addListener(_didChangeTextEditingValueListener); | |||
There was a problem hiding this comment.
It's looks like this PR removes the lines added by #2488 but keet some of the other changes, does this PR handle any related issues or conflicts with that PR? If no, then we need to revert that PR first, then merge this but revert the selection fix changes.
|
Read only mode needs the listener for selection changes. Only changes I see in my branch is controller.addListener(_didChangeTextEditingValueListener); and controller.removeListener(_didChangeTextEditingValueListener); are called in readonly mode. #2488 moves the handle focus change listener inside if (!widget.config.readOnly) {} - Could that have been the fix for focus changes bug? |
|
FYI - we've been internal testing with 20+ daily users for a couple of weeks with no issues. |
|
Thanks for your contribution. |
|
The default Android apps magnifier stays fixed to the text selection line, but this one moves freely with the finger's position. |
Description
Related Issues
Type of Change