fix: dragging fails for collapsed blocks with Icons, which have been … #6081

tweini · 2022-04-17T21:43:36Z

…deserialized from JSON

The basics

I branched from develop
My pull request is against develop
My code follows the style guide
I ran npm run format and npm run lint

The details

Fix for "Dragging fails to complete for collapsed procedure blocks that have been deserialised from JSON #6076".
The bug affect every block with icons, not only procedures. So comments in a collapsed block will trigger this bug too.
Precondition is a deserialization from JSON.

Resolves

bug #6076

Proposed Changes

Provides an appropriate check if the Icon is visible, as this guarantees an initialized state of the Icon Object belonging to the block.
Not moving the not visible Icons should not have side effects, as expanding the block will render the icons so they get a proper state.
An alternative solution would be a simple null check, but this could chew future errors. The specific checks are imho the better way.

Behavior Before Change

Behavior After Change

Reason for Changes

Test Coverage

See #6076 for manual test steps.

Tested on:

Chrome Desktop

Documentation

There is a code comment in the PR explaining the changes and mentions the # of the issue

Additional Information

BeksOmega · 2022-04-18T14:22:14Z

Hello! Thank you for the contribution :D Were you able to find out why this was happening with JSON but not with XML?

tweini · 2022-04-18T18:21:45Z

Hi Beka! Well, although I am a big fan of cereals, I didn't read your JSO serializer code, yet. Shall I?

BeksOmega · 2022-04-18T19:25:53Z

Hi Beka! Well, although I am a big fan of cereals, I didn't read your JSO serializer code, yet. Shall I?

I think that's a good idea if you have time =) If this is occuring with the JSON serializer but not with XML, I'm hoping it can be fixed at the serializer level, rather than here. (unless you think fixing the problem here is better because [for example] it will fix some other bugs)

tweini · 2022-04-19T04:58:08Z

Okidoki, I managed to track down the bug in the JSO serializer. It misses to call Icon.computeIconLocation() fyi: all calls to this method in entire codebase

Some call stacks:

Call stack when deserialized from xml:
computeIconLocation (icon.js:176)
moveConnections (block_svg.js:800)
moveBy (block_svg.js:473)
domToWorkspace (xml.js:471)
load (playground.html:173)

Call stack when deserialized from JSON:
initBlock (blocks.js:618)
appendPrivate (blocks.js:400)
appendInternal (blocks.js:345)
append (blocks.js:308)
load (blocks.js:683)
load (workspaces.js:89)
load (playground.html:170)

And now the fun begins. Here I would place the computeIconLocation()-calls for the icons:

blockly/core/serialization/blocks.js

Line 513 in d2329f8

// TODO(#2105): Remove this logic and put it in the icon.

#2105 tells about an Icon API. Is there anything in progress?

So, before providing a new PR, I thought it will be better to sort out what is the best way to do it.

I am fine with contributing, and I am happy digging into the Blockly codebase 😀

BeksOmega · 2022-04-19T16:12:04Z

Thank you so much for digging into this @tweini !

To make sure we're on the same page:

In the XML system, the icons are deserialized, and then later the block's position is deserialized. When the block's position is deserialized, it computes all of the locations for all of the icons.

In the JSON system, the block's position is desserialized, and then later the icons are deserialized. This means that the icon's positions never get computed, so we get errors when a drag occurs.

So to fix this for the JSON system, we just need to compute the icon location when it is deserialized.

Does that sound like what you're thinking?

#2105 tells about an Icon API. Is there anything in progress?

Nope! So you can make changes to the JSON serializer without worrying about conflicts =)

tweini · 2022-04-19T20:20:25Z

Pleasure is on my side @BeksOmega . Debugging is so much fun 😊 so I did further investigation.

The truth is not as simple as you proposes 😨 , so sorry.

Deserializing an Icon does not set proper location, so the .iconXY_ property remains null although the the icon object is created and attached to the block.

Setting the icon location is a side effect on both serializers. While xml deserialization, the icon is drawn (really appears on the workspace!) without location here:

blockly/core/block_svg.js

Line 290 in d2329f8

this.workspace.getCanvas().appendChild(svg);

This happens no matter the block is collapsed or not. If not collapsed, the icon gets its position with the call from the drawer L337 and is then properly drawn at its new position.

The uncollapsed block draws the icon as mentioned above. But its visibility state is maintained afterwards in block_svg L679 and then the drawer L111 hides it by setting display:none. The location at this point is null and it is set (as a side effect) in block_svg L800.

I am sure you are aware of the problems, as I visited two issues from you during my investigation. For now, this bug must be patched, and a proper call to .computeIconLocation() would raise awareness for the problem in the JSO serializer.

The handling of Icons causes horrible things (citation from #2105 😄 ), which can't be unseen. And I think it is touching issue #5146 as well as .initSvg() does the mentioned call (see above, L290).

BeksOmega · 2022-04-19T22:41:06Z

So what solution are you thinking of @tweini ?

tweini · 2022-04-20T01:50:19Z

I am sure you are aware of the problems, as I visited two issues from you during my investigation. For now, this bug must be patched, and a proper call to .computeIconLocation() would raise awareness for the problem in the JSO serializer.

Did change my PR with the new solution mentioned above. Hope my shared infos helps. I got a better understanding of the internals.

BeksOmega · 2022-04-20T14:06:21Z

I am sure you are aware of the problems, as I visited two issues from you during my investigation. For now, this bug must be patched, and a proper call to .computeIconLocation() would raise awareness for the problem in the JSO serializer.

Did change my PR with the new solution mentioned above. Hope my shared infos helps. I got a better understanding of the internals.

Sweet! I couldn't tell if by:

The truth is not as simple as you proposes 😨 , so sorry.

You meant it also needed a different solution hehe. Either way I really appreciate the added context about the internals!

BeksOmega · 2022-04-20T14:06:38Z

core/block_dragger.js

-      data.icon.setIconLocation(Coordinate.sum(data.location, dxy));
+      // Don't move hidden Icons as they can have not been initialized.
+      // fixes #6076
+      if (!this.draggingBlock_.isCollapsed() || !data.icon.collapseHidden) {


Do we still need this fix?

Nope, core/block_dragger.js remains untouched. I thought i got rid of the changes I made in my first commit 🤔

Wait! This code did trick my manual testing into false positive! More info in main thread... .

tweini · 2022-04-20T23:34:56Z

As mentioned, I thought this code was removed. The fix fails if the icon is a mutator, as the mutator isn't serialized in the icon state of the serialization object. so we have to move it to the end. PR follows...

BeksOmega · 2022-04-21T18:29:31Z

Heyo! So I think the issue with the build is that icons don't exist in headless mode (as discussed in #2105 (comment)) If you move the computeIconLocation calls to after this line I think it should work.

Thanks for catching that issue with mutators! I wouldn't have seen that from looking at the code.

tweini · 2022-04-21T19:17:10Z

Thanks for your suggestion, works like a charm. I ran the tests locally, and gladly the erroneous commit put this step back in scope 👓 . And I begin understanding what #2105 is about, as it is also referenced in you project cereal doc.
Hope all is fine, the next bug will be fixed later on. Was another deep dive into serialization internals 🤿

BeksOmega · 2022-04-21T21:01:34Z

Thank you again for the fix and all of the digging! :D

tweini · 2022-04-22T00:37:07Z

@BeksOmega I am very happy 😃 , short celebration 🥳 , opened next pull request to prepare more happiness 😆 (and parties 🎉 )

google#6081) * fix: 6076 "dragging fails" improved * fix: "dragging fails (bug google#6076)" tested version * fix: "dragging fails (bug google#6076)" moved fix-code after L625

tweini requested a review from a team as a code owner April 17, 2022 21:43

tweini requested a review from rachel-fenichel April 17, 2022 21:43

github-actions bot assigned rachel-fenichel Apr 17, 2022

fix: 6076 "dragging fails" improved

a0171b1

tweini force-pushed the fix-#6076-Dragging-fails-to-complete branch from bb155f2 to a0171b1 Compare April 20, 2022 01:41

BeksOmega reviewed Apr 20, 2022

View reviewed changes

BeksOmega removed the request for review from rachel-fenichel April 20, 2022 22:28

BeksOmega assigned BeksOmega and unassigned rachel-fenichel Apr 20, 2022

fix: "dragging fails (bug google#6076)" tested version

c89300e

fix: "dragging fails (bug google#6076)" moved fix-code after L625

c714f0b

BeksOmega approved these changes Apr 21, 2022

View reviewed changes

BeksOmega merged commit 5240301 into google:develop Apr 21, 2022

This was referenced Apr 21, 2022

chore(develop): release blockly 9.0.0 #6096

Closed

chore(develop): release blockly 9.0.0 #6100

Closed

chore(develop): release blockly 9.0.0 #6101

Closed

tweini deleted the fix-#6076-Dragging-fails-to-complete branch April 28, 2022 01:09

BeksOmega mentioned this pull request Apr 28, 2022

fix: JSON deserialization of certain collapsed blocks #6126

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: dragging fails for collapsed blocks with Icons, which have been … #6081

fix: dragging fails for collapsed blocks with Icons, which have been … #6081

tweini commented Apr 17, 2022 •

edited by BeksOmega

Loading

BeksOmega commented Apr 18, 2022

tweini commented Apr 18, 2022

BeksOmega commented Apr 18, 2022

tweini commented Apr 19, 2022

BeksOmega commented Apr 19, 2022

tweini commented Apr 19, 2022

BeksOmega commented Apr 19, 2022

tweini commented Apr 20, 2022

BeksOmega commented Apr 20, 2022

BeksOmega Apr 20, 2022

tweini Apr 20, 2022

tweini Apr 20, 2022

tweini commented Apr 20, 2022

BeksOmega commented Apr 21, 2022

tweini commented Apr 21, 2022

BeksOmega commented Apr 21, 2022

tweini commented Apr 22, 2022

fix: dragging fails for collapsed blocks with Icons, which have been … #6081

fix: dragging fails for collapsed blocks with Icons, which have been … #6081

Conversation

tweini commented Apr 17, 2022 • edited by BeksOmega Loading

The basics

The details

Resolves

Proposed Changes

Behavior Before Change

Behavior After Change

Reason for Changes

Test Coverage

Documentation

Additional Information

BeksOmega commented Apr 18, 2022

tweini commented Apr 18, 2022

BeksOmega commented Apr 18, 2022

tweini commented Apr 19, 2022

BeksOmega commented Apr 19, 2022

tweini commented Apr 19, 2022

BeksOmega commented Apr 19, 2022

tweini commented Apr 20, 2022

BeksOmega commented Apr 20, 2022

BeksOmega Apr 20, 2022

Choose a reason for hiding this comment

tweini Apr 20, 2022

Choose a reason for hiding this comment

tweini Apr 20, 2022

Choose a reason for hiding this comment

tweini commented Apr 20, 2022

BeksOmega commented Apr 21, 2022

tweini commented Apr 21, 2022

BeksOmega commented Apr 21, 2022

tweini commented Apr 22, 2022

tweini commented Apr 17, 2022 •

edited by BeksOmega

Loading