google / blockly

The web-based visual programming editor.
https://developers.google.com/blockly/
Apache License 2.0
12.39k stars 3.7k forks source link

Chasing reporter block around in zelos #7426

Closed rachel-fenichel closed 1 year ago

rachel-fenichel commented 1 year ago

Check for duplicates

Description

This bug was originally reported in #7412 by @clementcontet

See the screenshot for the originally reported bug.

I think the mechanism is: Sometimes when the insertion marker appears it moves the valid connection far enough away from the target connection that it becomes invalid, which makes the target block disconnect and get bumped away from the dragging block.

This is more likely to happen when using zelos because it changes the height of the input and moves the connection location down, while geras and thrasos leave the connection in the same location relative to the top left of the dragging block.

To verify: try the same drag with taller and shorter value blocks. The problem should become more visible the taller the value block is.

Reproduction steps

Stack trace

No response

Screenshots

https://github.com/google/blockly/assets/13686399/3c9ebe01-eb12-4388-bffb-f9bc81f424dc

Browsers

No response

clementcontet commented 1 year ago

Hi Rachel,

Indeed I don't have the problem with small child blocks, or when switching to Geras.

But I found that even when the connection is done directly (without "chasing"), when the insertion marker appears, it appears at a place that makes the child block to be moved below. Whereas in Geras, the insertion marker appears at the exact place that matches the child block current position.

https://github.com/google/blockly/assets/7261426/b1309d0b-dec1-47cf-9924-f00c822644ed

rachel-fenichel commented 1 year ago

The fix would be to note the difference between the position of the connection when the value input is expanded vs the position of the connection when the value input is at its default size, and to shift the parent bock up and left by that difference.

But this would cause a different problem: the dragged block would have to jump relative to the mouse. I think that would be worse. What do you think?

clementcontet commented 1 year ago

the dragged block would have to jump relative to the mouse

We would first see the insertion marker of the block on its target position, right? In that case, the "jump" would only occur when we drop the dragged block, and it would indeed be placed at the exact place of the insertion marker.

And I think we already see this "jump" with insertion marker:

https://github.com/google/blockly/assets/7261426/4fd56b87-3109-4655-9a2e-c47826310465

rachel-fenichel commented 1 year ago

You're correct. Okay, we'll look at getting the marker to position so that the connecting block doesn't have to move.

rachel-fenichel commented 1 year ago

@clementcontet If you want to fix this, take a look at the positionNearConnection function in block_svg.ts

clementcontet commented 1 year ago

Hi @rachel-fenichel, I tried to implement what you described, but I was a bit lost and I'm not sure I did well.

The fix would be to note the difference between the position of the connection when the value input is expanded vs the position of the connection when the value input is at its default size, and to shift the parent bock up and left by that difference

take a look at the positionNearConnection function in block_svg.ts

I didn't find a way to get the expanded input in positionNearConnection, so I had to implement something after the renderer has finished.

But if I only moved the insertion marker after the render, it didn't work either (I guess because the insertion marker would not be placed correctly next to the target connection and the renderer fails?).

So my solution was to keep intact the current move before the render, and after the render, move it again according the displacement of the expanded input.

Is it the right way to do it?

rachel-fenichel commented 1 year ago

Hi Clement,

Sorry for the delay in responding.

Yes, your solution looks reasonable (in this commit).

Your analysis is correct: the blocks have to be connected before you can figure out the new location of the connection, which means that repositioning has to happen after connecting instead of before. Doing it that way is also more general than the existing positionNearConnection method, so you should be able to remove positionNearConnection and have your new method handle all of the cases it handled.

To avoid flickering, you should also move the insertionMarker?.getSvgRoot().setAttribute('visibility', 'visible'); line to just after your resize, instead of just before.

If you put those changes into a PR, we'll happily review and take them.