Shadow block dropdown gets color of parent category

[clementcontet]

Check for duplicates

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

Description

If a shadow block has a dropdown, it gets the color of the parent category, instead of the color of the block own category.

Reproduction steps

1. Run npm run start and click the Advanced playground link to go to the advanced playground..
2. Load the following JSON into the playground:

{
  "blocks": {
    "languageVersion": 0,
    "blocks": [
      {
        "type": "controls_whileUntil",
        "inputs": {
          "BOOL": {
            "shadow": {
              "type": "logic_boolean"
            }
          }
        }
      }
    ]
  }
}

3. Click on the dropdown block.
4. Observe how the dropdown has the color of the parent.

Stack trace

No response

Screenshots

Tested on https://www.supercodingball.com/code/offline/do-nothing:

shadow.dropdown.color.mov

Browsers

Chrome desktop

To Fix

1. Run the reproduction steps to make sure you can reproduce the problem.
2. Modify the showEditor_ method in the FieldDropdown to always show the current block color (instead of showing the parent colour when the block is a shadow).
3. Rerun the reproduction steps and make sure you can't reproduce the problem..

[BeksOmega]

Related to RaspberryPiFoundation/blockly-samples#252

[laurensvalk]

Just adding a note that some builds may depend on the current behavior, at least in part.

It would be awesome if there was a config option (or renderer constant) to say whether shadow blocks should inherit the parent category styles or not. (Including dropdowns, fixing this issue in the process)

For example, it is not uncommon for blocks in different categories to have a text shadow block on various inputs. If I understand correctly, the way to do it currently is make multiple text block types, each with its own color. A custom renderer works for the most part, but still requires monkey-patching the borderrect of input fields.

I'll see if I can prepare a pull request to add a SHADOW_BLOCKS_USE_PARENT_STYLE = false option, if you think this makes sense.

[laurensvalk]

The current behavior was added here b038c1c#diff-8bc430f306f183ee3e55aa995c55063f906fe1ac043571db11d15b67566c3012, and seems like it might be intentional.

[maribethb]

@laurensvalk Thanks for reporting! Adding that option to the ConstantProvider class seems reasonable and we would take a PR adding it and updating the dropdown field to respect it. Thanks!

[clementcontet]

Hi,

From what I understand, shadow blocks current implemention gets the color from parent block, because we though that shadow blocks were often math blocks, and did not have a strong "meaning", justifying their own color.

On the other hand, in my example, the shadow block has a strong meaning (position in the football field) and having a consistent color for position blocks is important for users.

So it would be great indeed to tell Blockly if we want blocks to inherit their parent color or not.

But I think this distinction should not be done based on the status of the block (regular vs shadow).
If we simply set all shadow blocks to inherit their parent color:

• shadow math blocks will get the parent color, but regular math blocks won't
• this would not allow apps to deal at the same time with "strong meaning" shadow blocks and "weak meaning" shadow blocks

So another idea: could we attach this option (BLOCKS_USE_PARENT_STYLE) to the category of the block instead?
This way we could set the "math" category as inheritable (for both shadow and regular blocks), but not my "positions" category.

[laurensvalk]

Can you elaborate on strong/weak shadow blocks? I'm not sure I'm familiar with that distinction.

Would it work in your case to make the positions block not a shadow block, but non-movable and non-deletable instead? By nature of having a different color, maybe it doesn't need the darker shade to distinguish?

To keep things small, I think I'll start with the option at the constant provider level (and fix the original reported issue).

[laurensvalk]

@laurensvalk Thanks for reporting! Adding that option to the ConstantProvider class seems reasonable and we would take a PR adding it and updating the dropdown field to respect it. Thanks!

I started on this, with e.g. the dropdown fix below. It's not so bad for this one, but this is going to touch quite a bit of code if doing it like this everywhere (borderRect stroke, PathObject fill, etc.). Happy to keep going but I can imagine this is not great.

Would it make sense to add the shadow check to BlockSvg.getColourSecondary (and friends) instead, and in the fields call these instead of accessing this.style.colourSecondary directly? (Although maybe the block isn't available everywhere, like in PathObject.updateShadow_, so maybe also not the solution.) Or maybe a getter for the style object that takes the shadow state into account.

diff --git a/core/field_dropdown.ts b/core/field_dropdown.ts
index 0390a1031..9bb0cd837 100644
--- a/core/field_dropdown.ts
+++ b/core/field_dropdown.ts
@@ -279,13 +279,13 @@ export class FieldDropdown extends Field<string> {
     dom.addClass(menuElement, 'blocklyDropdownMenu');
 
     if (this.getConstants()!.FIELD_DROPDOWN_COLOURED_DIV) {
-      const primaryColour = block.isShadow()
-        ? block.getParent()!.getColour()
-        : block.getColour();
-      const borderColour = block.isShadow()
-        ? (block.getParent() as BlockSvg).style.colourTertiary
-        : (this.sourceBlock_ as BlockSvg).style.colourTertiary;
-      dropDownDiv.setColour(primaryColour, borderColour);
+      // On shadow blocks, optionally use parent style for dropdown.
+      const colorSource =
+        block.isShadow() && this.getConstants()!.SHADOW_BLOCKS_USE_PARENT_STYLE
+          ? block.getParent()
+          : block;
+      const style = (colorSource as BlockSvg).style;
+      dropDownDiv.setColour(style.colourPrimary, style.colourTertiary);
     }

[clementcontet]

Hi,

Can you elaborate on strong/weak shadow blocks? I'm not sure I'm familiar with that distinction.

By "strong" blocks, I meant blocks that represent a real concept in the app: in my game, a position on the football field.
And it is important to my audience (10+ kids) that all positions are represented by the same color (pink).

On the contrary, a "weak" block (which I don't use in the game), would be a technical category that does not represent an actual concept (like the number 10 in a "repeat 10 times" block).

With this distinction, strong blocks should have their own color whereas weak blocks should inherit from their parent (if any).
And I think we can't distinguish strong/weak blocks based on their status (regular vs shadow) but more based on their category (math/position/etc.)

Would it work in your case to make the positions block not a shadow block, but non-movable and non-deletable instead? By nature of having a different color, maybe it doesn't need the darker shade to distinguish?

Hi, indeed I don't use the darker shades for shadows.

I just want my "default" blocks to be:

• not deletable => so that users always see what kind of block is needed, based on their color
• replacable, obviously

And I think only shadow blocks are at the same time replacable and not deletable?

Maybe it's easier to try directly: https://www.supercodingball.com/code/offline/1player-only

[laurensvalk]

Thanks for clarifying! For what it's worth, the Zelos number shadow block does not show any base color, so this case might not be the biggest hurdle here.

If I understand correctly, the proposed approach would work for your app then by setting BLOCKS_USE_PARENT_STYLE to false (the default).

[clementcontet]

If I understand correctly, the proposed approach would work for your app then by setting BLOCKS_USE_PARENT_STYLE to false (the default).

Sure! I was just worried for other apps.

[Aradhya04]

Can you assign this to me