GrapesJS / grapesjs

Free and Open source Web Builder Framework. Next generation tool for building templates without coding
https://grapesjs.com
BSD 3-Clause "New" or "Revised" License
22.38k stars 4.06k forks source link

BUG: Flex Property not working in style manager #4116

Closed saudAtIrisdame closed 2 years ago

saudAtIrisdame commented 2 years ago

GrapesJS version

What browser are you using?

Version 1.34.81 Chromium: 97.0.4692.99 (Official Build) (64-bit)

Reproducible demo link

https://codesandbox.io/s/strange-sun-qgdcq?file=/index.js

Describe the bug

Flex Property

How to reproduce the bug?

  1. create custom type component
  2. drop the component in canvas
  3. set the display property to flex in style manager.

What is the expected behavior? Should show flex properties when the display(general property) is flex

What is the current behavior? Not showing flex properties

It is necessary to execute some code in order to reproduce the bug, paste it here below:

// your code here

Code of Conduct

Vac1911 commented 2 years ago

I diagnosed the issue after finding the same problem independently.

Looking at StyleManager.select()

https://github.com/artf/grapesjs/blob/75cd582a8d1a91096276bc4dccc7475a269ad45c/src/style_manager/index.js#L339-L395

On line 369, every property is looped through to check its visibility for the new target.

However the __checkVisibility function does not access the current value by looking at the target's styles. Instead it looks for the current value stored in the property model. (source)

The problem is that __checkVisibility is being called before the property model hasn't been told a new target has been selected, as that does not happen until line 392. So the value being checked is not from the target we just selected.

saudAtIrisdame commented 2 years ago

@Vac1911 @artf I got it, but what's the solution, how can I fix this problem? is this grapejs issue or mine?

Vac1911 commented 2 years ago

@saudAtIrisdame It seems to be a grapejs issue. I fixed it for myself changing the source code, reordering the select method to check visibility last.

Here is the dist file after running build:js https://gist.github.com/Vac1911/4c89f2a48809bee5fefd2d21525d37ad

   select(target, opts = {}) {
      const { em } = this;
      const trgs = isArray(target) ? target : [target];
      const { stylable } = opts;
      const cssc = em.get("CssComposer");
      let targets = [];

      trgs.filter(Boolean).forEach((target) => {
        let model = target;

        if (isString(target)) {
          const rule = cssc.getRule(target) || cssc.setRule(target);
          !isUndefined(stylable) && rule.set({ stylable });
          model = rule;
        }

        targets.push(model);
      });

      const component =
        opts.component || targets.filter((t) => isComponent(t)).reverse()[0];
      targets = targets.map((t) => this.getModelToStyle(t));
      const state = em.getState();
      const lastTarget = targets.slice().reverse()[0];
      const lastTargetParents = this.getParentRules(lastTarget, {
        state,
        component,
      });
      let stateTarget = this.__getStateTarget();

      // Handle the creation and update of the state rule, if enabled.
      em.skip(() => {
        if (state && lastTarget?.getState?.()) {
          const style = lastTarget.getStyle();
          if (!stateTarget) {
            stateTarget = cssc
              .getAll()
              .add({ selectors: "gjs-selected", style, important: true });
          } else {
            stateTarget.setStyle(style);
          }
        } else if (stateTarget) {
          cssc.remove(stateTarget);
          stateTarget = null;
        }
      });

      this.model.set({ targets, lastTarget, lastTargetParents, stateTarget });
      this.__upProps(opts);

      // Update sectors/properties visibility
      sectors.forEach((sector) => {
        const props = sector.getProperties();
        props.forEach((prop) => {
          const isVisible = prop.__checkVisibility({
            target: lastTarget,
            component,
            sectors,
          });
          prop.set("visible", isVisible);
        });
        const sectorVisible = props.some((p) => p.isVisible());
        sector.set("visible", sectorVisible);
      });

      return targets;
    },
artf commented 2 years ago

Thanks for the proper investigation @Vac1911 I'll fix it in the next release.