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

Shadow blocks inside statements with mutator input get rendered in insertion marker #7466

Closed
1 task done
laurensvalk opened this issue Sep 7, 2023 · 3 comments · Fixed by #7609
Closed
1 task done
Assignees
Labels
issue: bug Describes why the code or behaviour is wrong

Comments

@laurensvalk
Copy link
Contributor

laurensvalk commented Sep 7, 2023

Check for duplicates

  • I have searched for similar issues before opening a new one.

Description

When a mutator adds an input with a shadow block, it gets rendered in the statement insertion marker.

It's not just a visual glitch. It accepts connections, so it can turn into a real block. This makes it an orphaned shadow block, which causes a crash when clicked. It seems somewhat related to #7384 and the related recent fixes.

Reproduction steps

The following example is a bit contrived but offers a minimalist way to reproduce. See context below for a more sensible use case. This can be reproduced without the plugin as well; using it just provides a ready-made block with a mutator that exhibits the issue.

  1. Use blockly-samples/plugins/block-plus-minus
  2. Apply the following patch:
diff --git a/plugins/block-plus-minus/package.json b/plugins/block-plus-minus/package.json
index 6d74dd98..d7116253 100644
--- a/plugins/block-plus-minus/package.json
+++ b/plugins/block-plus-minus/package.json
@@ -41,13 +41,13 @@
   "devDependencies": {
     "@blockly/dev-scripts": "^2.0.1",
     "@blockly/dev-tools": "^7.0.3",
-    "blockly": "^10.0.0",
+    "blockly": "^10.1.3",
     "chai": "^4.2.0",
     "mocha": "^10.2.0",
     "sinon": "^9.0.1"
   },
   "peerDependencies": {
-    "blockly": "^10.0.0"
+    "blockly": "^10.1.3"
   },
   "publishConfig": {
     "access": "public",
diff --git a/plugins/block-plus-minus/src/if.js b/plugins/block-plus-minus/src/if.js
index 10f6cd24..3bb8ca16 100644
--- a/plugins/block-plus-minus/src/if.js
+++ b/plugins/block-plus-minus/src/if.js
@@ -142,6 +142,14 @@ const controlsIfMutator = {
         .appendField(Blockly.Msg['CONTROLS_IF_MSG_ELSEIF'])
         .appendField(
             createMinusField(this.elseIfCount_), 'MINUS' + this.elseIfCount_);
+
+    const input = this.getInput('IF' + this.elseIfCount_);
+    if (input.connection) {
+      const field = '<field name="BOOL">TRUE</field>';
+      const xml = `<shadow type="logic_boolean">${field}</shadow>`;
+      input.connection.setShadowDom(Blockly.utils.xml.textToDom(xml));
+    }
+
     this.appendStatementInput('DO' + this.elseIfCount_)
         .appendField(Blockly.Msg['CONTROLS_IF_MSG_THEN']);
  1. Install and run the demo for this plugin. The video illustrates the following steps.
  2. Attach the block to an existing statement on the workspace.
  3. Add some inputs using +. Set the bool value to FALSE to make the difference easier to see
  4. Drag it around. Observe shadow blocks rendered in the insertion marker (note that their value is TRUE).
  5. Drag it around more and catch the rendered shadow to become a real, orphaned shadow block.

context

The aforementioned demo isn't particularly useful but shadow blocks in mutator statements can have good use cases. Consider, for example:

  • a print block with a +/- like mutator that adds additional inputs, each with a text shadow block for easy text entry
  • a procedure caller with shadow inputs. See this link for context and a response from Neil.

context 2

Other than this issue, shadow block in mutators work great, so I'm hoping this is a valid use case. For some reason my own application does not reproduce step 7, so I have only the visual glitch.

on json vs xml

This seems unrelated because the same issue reproduces when replacing the patch above with:

    const input = this.getInput('IF' + this.elseIfCount_);
    if (input.connection) {
      // Create the new block.
      const boolBlock = Blockly.serialization.blocks.append(
          {
            type: 'logic_boolean',
          },
          this.workspace,
      );

      // Convert to shadow block and attach.
      boolBlock.setShadow(true);
      input.connection.connect(boolBlock.outputConnection);
    }

But this has a second issue: it renders visually different, and shows the bool block's border rect until you refresh the page to reload it as a normal shadow block. Edit: a render() after setShadow and before connect makes it look correctly (but the main issue persists). Edit2: setShadow should perhaps always call renderEfficiently.

Stack trace

No response

Screenshots

update-2023-09-07_09.02.33.mp4

Browsers

No response

@laurensvalk laurensvalk added issue: bug Describes why the code or behaviour is wrong issue: triage Issues awaiting triage by a Blockly team member labels Sep 7, 2023
@BeksOmega
Copy link
Collaborator

Currently you can check if the block isInsertionMarker() during deserialization and not add shadows if that's the case. But if we reimplement #7316 then it would become impossible (without additional changes), because isInsertionMarker would be set to true after the extra mutation state is deserialized.

@laurensvalk
Copy link
Contributor Author

Thanks for the hint! This gives a couple of variations. In the context of this example:

  1. The original case, which also triggers the bug above.
original-case-with-bug.mp4
  1. Don't insert the bool blocks if the if block is an insertion marker.

    This avoids the bug, but naturally the marker has a smaller shape now (which is OK here, but could be an issue). Or maybe this is the expected result because normal inputs also aren't rendered in the insertion marker?

dont-insert-if-parent-insertion-marker.mp4
  1. Insert the bool blocks, but set them to insertion marker when the if block is an insertion marker.

    Avoids the duplication issue and has the right shape. This seems to be the best for now.

set-bools-insertion-if-parent-insertion.mp4

But if we reimplement #7316 then it would become impossible (without additional changes), because isInsertionMarker would be set to true after the extra mutation state is deserialized.

Inspired by variant 2, perhaps child blocks could be set as insertion markers as well. (But somehow ensure that this is not applied to blocks already on the workspace, when considering an insertion marker that makes an outer connection).

@BeksOmega
Copy link
Collaborator

Inspired by variant 2, perhaps child blocks could be set as insertion markers as well. (But somehow ensure that this is not applied to blocks already on the workspace, when considering an insertion marker that makes an outer connection).

I think this is a good idea! Because this issue could also be triggered by people creating shadow blocks in their init method. It doesn't /just/ apply to mutators.

@rachel-fenichel rachel-fenichel added this to the Upcoming milestone Sep 13, 2023
@rachel-fenichel rachel-fenichel removed the issue: triage Issues awaiting triage by a Blockly team member label Sep 13, 2023
@maribethb maribethb removed this from the Upcoming milestone Sep 27, 2023
@BeksOmega BeksOmega self-assigned this Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: bug Describes why the code or behaviour is wrong
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants