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

feat(editor): kanban mobile drag and drop #9411

Open
wants to merge 12 commits into
base: canary
Choose a base branch
from

Conversation

alsuren
Copy link

@alsuren alsuren commented Dec 28, 2024

this is based on toeverything/blocksuite#9048

This PR includes:

  • A fix for when the drag and drop can get into a bad state after the browser sends us a pointercancel event
  • An implementation of drag and drop for mobile
    • kanban/mobile/controller/drag.ts is mostly copy-paste from kanban/pc/controller/drag.ts . Do you want me to DRY it out at all?
    • I have disabled the "mobile databases are readonly" logic from toeverything/blocksuite@d90ff22 until someone can give me a bit more context (I have asked in #technical-discussion on discord). If you have a suggested fix for this, please leave a comment (or just push your proposed fix to my branch).
  • I haven't worked out the best way to port my tests yet. Should I add the mobile configs to blocksuite/tests-legacy, or try to port them to tests/affine-mobile or somewhere? I've not worked out who to add a kanban view on affine yet, so this might take more time.

This is based on top of toeverything/blocksuite#9044 so it included a basic playwright test. I'm happy to merge this as two separate PRs or close the other one if you prefer.

TODO/known bugs from QAing (some for now, and some for fixing in later issues/PRs):

  • ignore drags if the user didn't stay in the same place for long enough (how long?) before moving
    • I guess I should display the tilted drag preview as soon as the timeout expires?
  • (later) the horizontal blue drop position indicator doesn't show if you're dropping into an empty column (this is also a problem on desktop affine.fail)
  • (later) it is possible to drop a card into the section below the kanban board and it turns it into a text card, losing information (I might split this out into its own issue though, because it is also a problem on desktop, and pressing undo will get it back where you started)
  • (later) clicking and dragging on the T causes it to render the tilted drag preview, but it stays where it is rather than following your cursor (this is also a problem on desktop affine.fail: it renders the previews as if you're moving the card successfully, but when you release it silently doesn't do anything)
  • (later?) sometimes after you've been playing about for a while, an invisible shows up and blocks all interactions with the page. It does not go away when you refresh the page, so you have to open a new browser tab to continue working. (I think this happens when you click any of the 3 dot menus - are they implemented as a pushstate() with hidden data sessionstorage or something? I am tempted to split out a separate PR to enable editing of mobile data views, because the 3dot menus aren't visible on read-only data views)
  • (later) sometimes after you've been playing around for a while, the drag-to-scroll behaviour breaks. It does not fix itself when you refresh the page, so you have to open a new browser tab to continue working. This feels like a chrome bug?

@alsuren alsuren requested a review from a team as a code owner December 28, 2024 14:35
@CLAassistant
Copy link

CLAassistant commented Dec 28, 2024

CLA assistant check
All committers have signed the CLA.

@graphite-app graphite-app bot requested a review from forehalo December 28, 2024 14:35
Copy link

graphite-app bot commented Dec 28, 2024

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@Saul-Mirone Saul-Mirone requested a review from zzj3720 December 28, 2024 17:19
Copy link

codecov bot commented Dec 30, 2024

Codecov Report

Attention: Patch coverage is 9.09091% with 110 lines in your changes missing coverage. Please review.

Project coverage is 52.53%. Comparing base (8d269c8) to head (5809fbf).
Report is 44 commits behind head on canary.

Files with missing lines Patch % Lines
.../src/view-presets/kanban/mobile/controller/drag.ts 8.84% 103 Missing ⚠️
...ffine/data-view/src/view-presets/kanban/pc/card.ts 0.00% 3 Missing ⚠️
blocksuite/affine/data-view/src/core/utils/drag.ts 0.00% 2 Missing ⚠️
...view/src/view-presets/kanban/mobile/kanban-view.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           canary    #9411      +/-   ##
==========================================
+ Coverage   51.67%   52.53%   +0.85%     
==========================================
  Files        2152     2152              
  Lines       97009    97103      +94     
  Branches    16205    16163      -42     
==========================================
+ Hits        50133    51010     +877     
+ Misses      45513    44680     -833     
- Partials     1363     1413      +50     
Flag Coverage Δ
server-test 78.14% <ø> (+<0.01%) ⬆️
unittest 32.55% <4.95%> (+1.47%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@zzj3720 zzj3720 changed the title Kanban mobile drag and drop. feat(editor): kanban mobile drag and drop. Dec 30, 2024
@zzj3720
Copy link
Member

zzj3720 commented Dec 30, 2024

Thank you for your pull request!

To enhance the development experience and code maintainability, we should consider generalizing the current drag-and-drop functionality to support long-press dragging on mobile devices. At present, the key difference between mobile and PC drag-and-drop is the initiation: long-press on mobile versus click on PC. We can make the code adaptable for both platforms.

Previously, the mobile version was set to readonly due to some database issues. Now that these issues have been resolved, we can safely remove this restriction.

Moreover, to ensure consistency and maintainability, please avoid adding new test code to blocksuite/tests-legacy. Instead, it would be more efficient to integrate blocksuite’s test code with AFFiNE's existing test framework.

Thank you for your efforts and understanding!

@alsuren
Copy link
Author

alsuren commented Dec 30, 2024

To enhance the development experience and code maintainability, we should consider generalizing the current drag-and-drop functionality to support long-press dragging on mobile devices. At present, the key difference between mobile and PC drag-and-drop is the initiation: long-press on mobile versus click on PC. We can make the code adaptable for both platforms.

I haven't explicitly implemented the long press thing. I just make all touch events on cards route through the same logic as the desktop version. We are just using pointer events, rather than the web platform's native drag and drop.

If we want to emulate the web platform's native long-press drag and drop behaviour, we would need to add extra logic for that. I have chosen not to do this because:

  1. it's fiddly and hard
  2. I'm not sure how to say touch-action: none and then translate non-long-press events back into scroll events
  3. long press on chrome brings up the context menu by default on mobile (and on desktop Chrome when in mobile device emulation mode). We would need to disable this to make testing possible.

I'm not using the web drag and drop functionality because of 3 and because it's not implemented on mobile Firefox (Which is the browser I use)

Previously, the mobile version was set to readonly due to some database issues. Now that these issues have been resolved, we can safely remove this restriction.

That's fantastic news. I will clean that up.

Moreover, to ensure consistency and maintainability, please avoid adding new test code to blocksuite/tests-legacy. Instead, it would be more efficient to integrate blocksuite’s test code with AFFiNE's existing test framework.

Okay. I'll have another go at that.

I noticed in passing that the affine playwright tests take a lot longer per test than the blocksuite ones (I may have picked an unlucky set of tests though, or imagined it). Should I try to write these as vitest tests instead, or is that going to be too much pain?

[Sorry for the edit. I pressed the send button by accident]

@github-actions github-actions bot added the test Related to test cases label Dec 30, 2024
@alsuren alsuren changed the title feat(editor): kanban mobile drag and drop. feat(editor): kanban mobile drag and drop Jan 3, 2025
@zzj3720
Copy link
Member

zzj3720 commented Jan 3, 2025

  1. I'm not sure how to say touch-action: none and then translate non-long-press events back into scroll events
  2. long press on chrome brings up the context menu by default on mobile (and on desktop Chrome when in mobile device emulation mode). We would need to disable this to make testing possible.

I apologize for the delayed response; I've been on vacation recently.

Regarding the implementation of long-press dragging, this feature is essential for mobile devices as defined by the product design. If it cannot be implemented at the moment, we are open to temporarily not having the drag-and-drop functionality on mobile, which means we won't need to adjust the drag-and-drop feature for mobile devices for now.

About the issue of long-press triggering the context menu, perhaps we could listen for the contextmenu event to prevent the default browser behavior. For simulating the long-press event, we might consider listening for the pointerdown event followed by the pointermove. If the touch does not move beyond a certain distance—though implementing this user experience enhancement can be secondary—we could trigger the drag action; otherwise, this pointerdown would not initiate a drag.

Thank you for your understanding!

@alsuren
Copy link
Author

alsuren commented Jan 3, 2025

I apologize for the delayed response; I've been on vacation recently.

I completely understand. This is the curse of being an open source maintainer: you get a bunch of drive by contributors when everyone is taking time off, and that's exactly when you want to be taking time off.

Regarding the implementation of long-press dragging, this feature is essential for mobile devices as defined by the product design.

Thanks for your guidance so far. I have learned a lot already.

Do you have design documents that can be shared/screenshotted? I was hoping that I could get away with shipping a half-baked solution to unblock my use-case (sharing a kanban board with my dad, to help me with job hunting). Do I also need to add a drag handle to help with accessibility?

otherwise, this pointerdown would not initiate a drag.

Is it okay for this pointerdown to also not cause the page to scroll? (This is what happens if we have touch-events: none. You need to avoid putting your thumb on a card if you want to start scrolling, because the browser will swallow+ignore your touch of you do)

How evergreen can we be about our browser requirements? If long-press is a hard requirement then I'm almost tempted to implement it in terms of
https://web.dev/articles/drag-and-drop (and only use the pointer events logic if that is not supported).

For now, I will try implementing the long press behaviour as you suggested. Thanks again for your feedback.

@alsuren alsuren force-pushed the kanban-mobile-drag branch from 103c458 to ad6282c Compare January 4, 2025 18:21
@alsuren
Copy link
Author

alsuren commented Jan 4, 2025

Regarding the implementation of long-press dragging, this feature is essential for mobile devices as defined by the product design

Done. It wasn't as painful as I expected, once I found blocksuite/framework/block-std/src/event/control/pointer.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Related to test cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants