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

Basic touchscreen support (#147) and rewrite of coordinate management #148

Merged
merged 11 commits into from
Jul 17, 2024

Conversation

FelixZY
Copy link
Contributor

@FelixZY FelixZY commented Jun 26, 2024

See #147. Also fixed some minor bugs that were annoying me.

Fixes #147

Copy link

vercel bot commented Jun 26, 2024

@FelixZY is attempting to deploy a commit to the dottle's projects Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Member

@1ilit 1ilit left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

When the zoom is anything but 1 the red dashed line for linking fields does not end on the cursor. Could you please check it out?

Other than that everything looks great

@FelixZY
Copy link
Contributor Author

FelixZY commented Jun 27, 2024

@1ilit I'll look into it but it will probably be a few days before I'm able.

@1ilit
Copy link
Member

1ilit commented Jun 27, 2024

@FelixZY no worries take your time

@FelixZY
Copy link
Contributor Author

FelixZY commented Jun 30, 2024

@1ilit Try it out now. I still need to tidy up a bit (git rebase -i --autosquash main, write a proper commit message on the last commit, figure out what to do with my type declarations) but the code should be reviewable from a functionality standpoint.

For the future: please consider migrating to Typescript.

@FelixZY
Copy link
Contributor Author

FelixZY commented Jul 12, 2024

@1ilit the branch is now cleaned up, rebased and ready to review, test and merge!

@FelixZY FelixZY requested a review from 1ilit July 12, 2024 18:08
@FelixZY FelixZY changed the title Basic touchscreen support (#147) and minor bugfixes Basic touchscreen support (#147) and rewrite of coordinate management Jul 12, 2024
@FelixZY
Copy link
Contributor Author

FelixZY commented Jul 13, 2024

I added a jitter fix as well. It's based on @l123wx's fix in #93 but less intrusive and hopefully more easily understood.

FelixZY added 5 commits July 14, 2024 15:36
Some mobile browsers (e.g. chrome) uses collapsing url bars (the
bar collapses when you scroll). In such cases, `100vh` typically
refers to the full height of the viewport when the url bar is
collapsed (see also `svh`, `lvh` and `dvh`, e.g. at
<https://ishadeed.com/article/new-viewport-units/#the-small-large-and-dynamic-viewport-units>
). This meant that on my tablet, the editor would extend below the
visible page until I scrolled it into view.

This commit re-uses a fix from some of my other projects
(specifically
<https://github.com/dansdata-se/api/blob/5c7e788d40006097a258be5dbfe456cb260fa25f/src/styles/globals.css#L14-L28>
) where the root element is set to fill 100% height. This avoids
dealing with viewport units altogether. On my tablet, this means
that the url bar is visible and that the editor does not extend
below the visible page.
This is basically a migration from mouse events to
[pointer events](
  https://developer.mozilla.org/en-US/docs/Web/API/Pointer_events
).

The `PointerEvent` interface inherits all of the `MouseEvent`
properties, meaning that existing code can essentially be left
as-is. The only major change is making sure we only respond to the
"primary" pointer.

Known issues include:
* stylus hover is not detected
* touchscreens do not have a concept of hover, making it difficult
  to e.g. resize areas
* no touch gesture support, e.g. "pinch-to-zoom"
During testing I accidentally managed to submit `NaN` as a pan
coordinate. This had the unfortunate side effect of bricking the
editor.

Given the serverity of an accidental `NaN` and that `NaN`s are not
impossible considering the amount of math involved in mouse move
operations, this commit introduces a simple validation step.

The new validation step should additionally be able to unstuck
anyone who have happened into this state by accident already.
Makes debugging issues in conversion from screen space
to diagram space easier.

Only adding english translations as I do not speak the
other languages.
@FelixZY
Copy link
Contributor Author

FelixZY commented Jul 14, 2024

Rebased on main and added scroll wheel panning. I'm not planning on adding more features 😅

@newpgit
Copy link

newpgit commented Jul 16, 2024

Rebased on main and added scroll wheel panning. I'm not planning on adding more features 😅

Hello, I tried the tablet movement sensitivity is very high, move a little bit of view to move a large distance, if you can add double finger zoom

你好,我试了一下 平板移动灵敏度很高,移动一点点视图就移动很大的距离,如果可以的话可以加上双指缩放吗

@newpgit
Copy link

newpgit commented Jul 16, 2024

Rebased on main and added scroll wheel panning. I'm not planning on adding more features 😅

836dd9fc41e4abdd686b1dd8cebc44ff.mp4

@FelixZY
Copy link
Contributor Author

FelixZY commented Jul 16, 2024

@newpgit I have not added support for gestures at this time - though I'm considering doing so after this PR has been accepted. Zoom gestures should not be working at this time.

the tablet movement sensitivity is very high, move a little bit of view to move a large distance

What type of interaction are you referring to? The screencast is really appreciated! However, I'm unable to see your touch points and am therefore unable to infer your intended actions. Are you experiencing that the view is moving faster than you are panning?

@newpgit
Copy link

newpgit commented Jul 17, 2024

@newpgit I have not added support for gestures at this time - though I'm considering doing so after this PR has been accepted. Zoom gestures should not be working at this time.

the tablet movement sensitivity is very high, move a little bit of view to move a large distance

What type of interaction are you referring to? The screencast is really appreciated! However, I'm unable to see your touch points and am therefore unable to infer your intended actions. Are you experiencing that the view is moving faster than you are panning?

Sorry, the previous video indeed didn't show the issue. Here's a new recording I've video.

fe876383aa8f28a242783805c078b878.mp4

@FelixZY
Copy link
Contributor Author

FelixZY commented Jul 17, 2024

@newpgit thank you very much - that is indeed strange. I'll take a look soon.

@newpgit
Copy link

newpgit commented Jul 17, 2024

@newpgit thank you very much - that is indeed strange. I'll take a look soon.

ok😄

@FelixZY
Copy link
Contributor Author

FelixZY commented Jul 17, 2024

@newpgit I have tested the following:

  • Google Chrome on Linux Desktop (with and without touch emulation) (Chromium/Blink)
  • Firefox on Linux Desktop (with and without touch emulation) (Gecko)
  • GNOME Web/Epiphany on Linux Desktop (WebKit)
  • Google Chrome on android 14 tablet (with touch and stylus)
  • Firefox on android 14 tablet (with touch and stylus)
  • Microsoft Edge on android 14 tablet (with touch and stylus)

Unfortunately, I'm unable to reproduce your results.

It looks like you are using an iPad in the recording - unfortunately I do not have access to one. I have seen from previous issues (e.g. #46 and #47) that safari has (for quite some time!) some issues with rendering things in the correct position (https://bugs.webkit.org/show_bug.cgi?id=23113). I can't say for sure that's what you are running into but that could be a possible explanation.

What I do see in your video is that the viewbox itself appears to change size as you move tables around - that definitely should not happen (and indeed does not on my devices).

@newpgit
Copy link

newpgit commented Jul 17, 2024

@newpgit I have tested the following:

  • Google Chrome on Linux Desktop (with and without touch emulation) (Chromium/Blink)
  • Firefox on Linux Desktop (with and without touch emulation) (Gecko)
  • GNOME Web/Epiphany on Linux Desktop (WebKit)
  • Google Chrome on android 14 tablet (with touch and stylus)
  • Firefox on android 14 tablet (with touch and stylus)
  • Microsoft Edge on android 14 tablet (with touch and stylus)

Unfortunately, I'm unable to reproduce your results.

It looks like you are using an iPad in the recording - unfortunately I do not have access to one. I have seen from previous issues (e.g. #46 and #47) that safari has (for quite some time!) some issues with rendering things in the correct position (https://bugs.webkit.org/show_bug.cgi?id=23113). I can't say for sure that's what you are running into but that could be a possible explanation.

What I do see in your video is that the viewbox itself appears to change size as you move tables around - that definitely should not happen (and indeed does not on my devices).

Yes, I have no problem using my phone. It seems like only the iPad has this issue.

Copy link
Member

@1ilit 1ilit left a comment

Choose a reason for hiding this comment

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

Great work! Love the new additions. Just a few edits that should be done.

In addition to the comments, connecting the columns on mobile doesn't seem to work. Could you please check that out as well?

Untitled.design.mp4

If you have any questions or objections don't hesitate to reach out.

Thank you:)

src/components/EditorHeader/ControlPanel.jsx Outdated Show resolved Hide resolved
src/context/TransformContext.jsx Show resolved Hide resolved
src/components/EditorCanvas/Canvas.jsx Outdated Show resolved Hide resolved
src/context/CanvasContext.js Outdated Show resolved Hide resolved
src/context/CanvasContext.js Outdated Show resolved Hide resolved
FelixZY added a commit to FelixZY/drawdb that referenced this pull request Jul 17, 2024
…t pattern

As noted [in the review](
drawdb-io#148 (comment)
), I disagree with this pattern. However, I will follow it to get
these features added.
Copy link

vercel bot commented Jul 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
drawdb ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 17, 2024 8:41pm

1ilit pushed a commit that referenced this pull request Jul 17, 2024
…t pattern

As noted [in the review](
#148 (comment)
), I disagree with this pattern. However, I will follow it to get
these features added.
FelixZY added 6 commits July 17, 2024 22:22
After some initial smaller fixes, it turned out that I had broken
the red line used when linking fields. Fixing this was not trivial
as I found myself battling a lot of small bugs relating to scale
and translation in the existing code. This was made extra difficult
as a lot of coordinates were calculated when necessary in
Canvas.jsx.

This commit attempts to simplify the coordinate management in a few
different ways:
* There are now two distinct coordinate systems in use, typically
  referred to as "spaces". Screen space and diagram space.
* Diagram space is no longer measured in pixels (though the
  dimension-less measure used instead still maps to pixels at 100%
  zoom).
* The canvas now exposes helper methods for transforming between
  spaces.
* Zoom and translation is now managed via the svg viewBox property.
  * This makes moving items in diagram space much easier as the
    coordinates remain constant regardless of zoom level.
* The canvas now wraps the current mouse position in a context
  object, making mouse movement much easier to work with.
* The transform.pan property now refers to the center of the screen.

A new feature in this commit is that scroll wheel zoom is now based
on the current cursor location, making the diagram more convenient
to move around in.

I have tried to focus on Canvas.jsx and avoid changes that might be
desctructive on existing save files. I also believe more refactors
and abstractions could be introduced based on these changes to make
the diagram even easier to work with. However, I deem that out of
scope for now.
This is similar to tools like figma, where the scroll wheel pans
the view and scroll is only done if the control key is pressed.

New bindings:
scroll wheel: pan y
shift + scroll wheel: pan x
ctrl + scroll wheel: zoom
Certain input sources (such as touch) are "captured" when they
press an element. This means the pointer is always considered
"inside" the element by the browser, even when they visually are
not. This caused some issues on mobile browsers where touch and
stylus events could not connect table columns with each other.

Just to be safe, I've added the required `releasePointerCapture`
call everywhere `onPointerEnter` or `onPointerLeave` is used.
…t pattern

As noted [in the review](
drawdb-io#148 (comment)
), I disagree with this pattern. However, I will follow it to get
these features added.
@1ilit 1ilit merged commit 4d0983b into drawdb-io:main Jul 17, 2024
4 checks passed
@FelixZY FelixZY deleted the fzy/mobile/1 branch July 17, 2024 20:42
@1ilit 1ilit mentioned this pull request Jul 28, 2024
sunrisedev99 pushed a commit to sunrisedev99/designdb--react that referenced this pull request Jul 28, 2024
…t pattern

As noted [in the review](
drawdb-io/drawdb#148 (comment)
), I disagree with this pattern. However, I will follow it to get
these features added.
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.

Touchscreen support
3 participants