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

fix: ignore backspace on empty table cells to avoid editor crash #964

Merged

Conversation

saif-ellafi
Copy link
Contributor

@saif-ellafi saif-ellafi commented Nov 7, 2024

Pressing "delete" (aka Backspace) on Android crashes the editor when called inside the empty cell of a table. It leaves the table in an invalid state, crashing the Editor.

This fix or workaround ignores backspaces called inside table cells at the first offset position of the cursor.

Any suggestions welcome!

@rileyhawk1417
Copy link
Contributor

rileyhawk1417 commented Nov 19, 2024

Hi @saif-ellafi seems like the fix works. Although table editing is not supported on mobile yet. The fix does help for some people who would manually want to bring the table editing experience to mobile.

I had tested before the fix and after.

Before (this was done on the release version of AppFlowy 0.7.3(2907301))

table_cells_delete_breaking_android.mp4

After (this was done on AppFlowy-Editor with the example file for desktop)

table_cells_delete_breaking_pr_964.mp4

@LucasXu0 do you mind confirming if this fix has already been implemented yet? Not sure how far along mobile support for table editing is.

@rileyhawk1417
Copy link
Contributor

I did run the tests locally on Arch Linux no errors happened although not sure why the CI test failed.

@saif-ellafi
Copy link
Contributor Author

Hello @rileyhawk1417 - thanks so much for checking out.

Yes with my fixes (also the other PRs are very important), I made it to production for my app and users are fine with it. I have added table controls in the toolbar to add and remove rows and columns and pretty much successful. It doesn't work with the in-place controls like desktop but honestly I find it even better this way through toolbar controls.

Best regards

@LucasXu0
Copy link
Collaborator

@saif-ellafi I can confirm that it will fix the table issue. Can you format your code to pass the CI?

@saif-ellafi
Copy link
Contributor Author

Hi @LucasXu0 - will take a look but the errors say absolutely nothing, apparently failing in the Ubuntu tests, could it be a random error? will run tests locally on my Ubuntu and see.

...
Downloading packages...
Got dependencies in `./example`.
Analyzing appflowy-editor...                                    
No issues found! (ran in 23.2s)
Formatted lib/src/editor/editor_component/service/ime/delta_input_on_delete_impl.dart
Formatted 592 files (1 changed) in 1.74 seconds.
Error: Process completed with exit code 1.

Copy link

codecov bot commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.24%. Comparing base (e7d9560) to head (f38285d).
Report is 15 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #964      +/-   ##
==========================================
+ Coverage   72.07%   72.24%   +0.17%     
==========================================
  Files         318      318              
  Lines       15074    15119      +45     
==========================================
+ Hits        10864    10923      +59     
+ Misses       4210     4196      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@saif-ellafi
Copy link
Contributor Author

Thank you @LucasXu0

@LucasXu0 LucasXu0 merged commit 09f4b17 into AppFlowy-IO:main Nov 27, 2024
7 checks passed
LucasXu0 added a commit to LucasXu0/appflowy-editor that referenced this pull request Nov 29, 2024
LucasXu0 added a commit that referenced this pull request Dec 2, 2024
* fix: hide the toolbar when all selected content is empty

* test: hide the toolbar when all selected content is empty

* Revert "fix: ignore backspace on empty table cells to avoid editor crash (#964)"

This reverts commit 09f4b17.
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