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

Our selection handling regressed #6012

Closed
maltenuhn opened this issue Jun 20, 2024 · 1 comment · Fixed by #6128
Closed

Our selection handling regressed #6012

maltenuhn opened this issue Jun 20, 2024 · 1 comment · Fixed by #6128
Assignees
Labels
Canvas Affects the visual editor it's a trap Good for newcomers Repro Hard Hard to reproduce the bug

Comments

@maltenuhn
Copy link
Member

maltenuhn commented Jun 20, 2024

Problems
1: In a number of cases, you now need to click (instead of mouse down) to select; or cmd-click twice on the canvas to make a selection, even when the hover highlight is drawn correctly

  1. click-and-drag now frequently does not work (see https://monosnap.com/file/C5GmtoFz9WOyqnOmbCmT22ty9hRp1N)

Solution Notes
We had quite fine-grained handling to cover on-canvas selection, including lots of edge cases (mouse-down-and-drag, cases where a click not a mouse down is required - in flex, if memory serves), and some of this was also refined when we introduced canvas strategies. If you find a solution that's deceptively easy, that probably only means it has regressed other behaviours other behaviours. Please talk about what you find, and the solutions you propose.

@maltenuhn maltenuhn added Canvas Affects the visual editor it's a trap Good for newcomers Repro Hard Hard to reproduce the bug labels Jun 20, 2024
@seanparsons seanparsons self-assigned this Jul 3, 2024
@seanparsons
Copy link
Contributor

@maltenuhn

Slight correction, it's not so much the click per se, but the mouse up that triggers some selection (especially in the cmd modified version of that). That was definitely an intentional change at some point in history.

The getPreferredSelectionForEvent function is likely responsible for the change in behaviour relating to cmd-double-clicking which also prevents selection from changing when elements are overlapping.

Cmd-dragging explicitly protects against changing selection during the mouse up.

Questions:

  • Do we want to operate with the axiom that whatever is highlighted will be selected on a mouse down (similarly for cmd mouse down)?
  • Should we never change selection on mouse up?
  • Should we have a way of locking selection from changing during an entire drag operation?

@seanparsons seanparsons removed their assignment Jul 4, 2024
@gbalint gbalint self-assigned this Jul 24, 2024
@gbalint gbalint mentioned this issue Jul 25, 2024
2 tasks
gbalint added a commit that referenced this issue Jul 29, 2024
**Problem:**

1. In a selected scene, dragging an element should drag the scene not
the element
2. In a selected scene, clicking on an element should select the element
3. In a selected scene, dragging an element under the threshold not move
anything, but select the element

4. In a selected scene, cmd-dragging an element should drag the element
not the scene
5. In a selected scene, cmd-clicking an element should select the
element
6. In a selected scene, cmd-dragging an element under the drag threshold
should not move anything, but select the element

**Fix:**
To fix 1-2-3 we need to make sure that dragging below the threshold is
handled as a "click", so the selection happens on mouse up.

To fix 4-5-6 we need to make sure the cmd-mousedown already selects the
element, which guarantees that the following dragging operates on the
element (and not the scene)

Meanwhile, the failing tests uncovered that we have an extra bug: you
can deselect a selected item if you click on a non-visible (e.g.
cropped) part of it. To fix it I modified the behavior of
`findValidTarget` in `dont-prefer-selected` mode: it means we should
change our selection to something different if possible, but we should
still not deselect it. To make this clearer I renamed
`don't-prefer-selected` to `prefer-more-specific-selection`

As I fixed the tests I had to realize that our dragging helper functions
are not good enough: we can specifiy modifier keys, but we can not
specify separate modifier keys for the mouse down and for the mouse move
events. Which is necessary because e.g. cmd changes the behavior
differently in the two cases: cmd on mouse down changes how selection
work, so it changes what will be dragged, while cmd on mouse move
changes which strategy will be applied (cmd allows reparenting).

**Manual Tests:**
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Preview mode

Fixes #6012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Canvas Affects the visual editor it's a trap Good for newcomers Repro Hard Hard to reproduce the bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants