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][web] Defer to browser toolbar and magnifier (Resolves #1390) #1483

Merged

Conversation

angelosilvestre
Copy link
Collaborator

[SuperEditor][web] Defer to browser toolbar and magnifier. Resolves #1390

On web, both our toolbar and the browser text selection toolbar are displayed.

This PR changes SuperEditor to avoid rendering the toolbar and drag handles on web on both Android and iOS:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-09-26.at.19.58.22.mp4

However, the Android drag handles aren't positioned exactly at the correct position.

android.webm

@matthew-carroll
Copy link
Contributor

@Jethro87 please take a look at the Android handle misalignment and confirm with us one more time that this is what you want for the web experience.

@Jethro87
Copy link
Contributor

Jethro87 commented Oct 4, 2023

@matthew-carroll I've reviewed #1390 and wanted to clarify the current options.

If we defer to browser controls, we are able to:

  • Get a native feel
  • Show the IME panels (if applicable)

at the cost of:

  • Grey box on mobile iOS
  • Slightly mispositioned selection handles on mobile Android

@angelosilvestre said this in the other issue:

If we don't need to interact with IME panels on mobile web, another option we have is to avoid reporting visual information (size, transform and text style) to the IME on mobile web. That way, the browser toolbar won't be visible and we won't have those grey boxes.

Does this method also hide the browser's selection handles, magnifier, etc? Previously, it was said that we would have to use BrowserContextMenu.disableContextMenu to disable the context menu. I don't think that's the case based on later comments, but I wanted to confirm.

Perhaps the best way forward would be to demo using super_editor's toolbar/magnifier/selection handles. I don't have full clarity on the tradeoffs and costs of using our vs the browser's.

@matthew-carroll
Copy link
Contributor

@angelosilvestre can you answer Jeff's question about hiding the handles and magnifier?

@Jethro87 you mentioned demo'ing Super Editor. Are you asking for a video of the same experiences but with Super Editor overlay controls instead of browser controls?

@Jethro87
Copy link
Contributor

Jethro87 commented Oct 4, 2023

@matthew-carroll Ya that's what I meant, just to ensure there's nothing amiss with super editor / mobile web.

@matthew-carroll
Copy link
Contributor

@angelosilvestre can you also add similar videos to the ones you've posted, but using the built-in overlay controls instead of deferring to web?

@angelosilvestre
Copy link
Collaborator Author

@matthew-carroll This video was recorded without reporting visual information to the IME. On iOS, the browser toolbar is still displayed in some moments. We could try to call BrowserContextMenu.disableContextMenu in the places where we should the editor toolbar.

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-10-04.at.19.14.43.mp4
android.webm

@Jethro87
Copy link
Contributor

Jethro87 commented Oct 6, 2023

@angelosilvestre What are the consequences of calling BrowserContextMenu.disableContextMenu? In #1390 you wrote

Flutter has BrowserContextMenu.disableContextMenu to disable the context menu for the whole app, but this isn't ideal as we don't want to force users to disable it for the entire application. Also, calling disableContextMenu doesn't seem to prevent the toolbar from being displayed.

That makes it sound like we don't want to go this route.

@angelosilvestre
Copy link
Collaborator Author

@Jethro87 We would call BrowserContextMenu.disableContextMenu and re-enable at the end of the frame. I'm not sure yet if it will work, but I could try.

@angelosilvestre
Copy link
Collaborator Author

Update: we would need flutter/flutter#134358 to be fixed to try the solution I proposed before.

@Jethro87
Copy link
Contributor

Jethro87 commented Oct 9, 2023

@angelosilvestre Right. It seems then that we need to use the browser's controls.

@matthew-carroll
Copy link
Contributor

@angelosilvestre @Jethro87 I've been trying to follow this thread, but I'm a bit unclear on the current expected output.

Can one or both of you state where you believe that we stand right now? What does Super Editor currently do in the relevant area, and what do we want Super Editor to do in the relevant area?

@Jethro87
Copy link
Contributor

Jethro87 commented Oct 9, 2023

@matthew-carroll Now that we have demos of both, my current understanding is that we must use the browser's toolbar/controls on mobile web and accept the trade offs (grey box on iOS and mispositioned handles on Android).

@angelosilvestre if I'm wrong please let me know.

@angelosilvestre
Copy link
Collaborator Author

@matthew-carroll If we should go in that direction, then this PR is ready for review.

@@ -1306,14 +1307,17 @@ class _AndroidDocumentTouchEditingControlsState extends State<AndroidDocumentTou
// Build the caret
_buildCaret(),
// Build the drag handles (if desired)
..._buildHandles(),
if (!isWeb) //
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add appropriate comments to each of these conditionals that explains why we're doing something special for web.

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.

Copy link
Contributor

@matthew-carroll matthew-carroll left a comment

Choose a reason for hiding this comment

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

LGTM

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][web] Browser text selection toolbar is displayed
3 participants