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

[BUG] Wrong position of toolbar when duplicate children component #2656

Closed
abozhinov opened this issue Mar 16, 2020 · 11 comments
Closed

[BUG] Wrong position of toolbar when duplicate children component #2656

abozhinov opened this issue Mar 16, 2020 · 11 comments

Comments

@abozhinov
Copy link

Hi,
you can check this example: https://codepen.io/abozhinov/pen/XWbqjEJ

Steps to reproduce the problem:

  1. If components existing delete them all.
  2. Drag a new carousel block.
  3. Select the first children's container.
  4. Click on the DUPLICATE icon to create a new component.
  5. After that try to select the newest component and then select the first component again.
  6. The position of the toolbar is wrong because the browser can't get the width and height of the element.

[Error] TypeError: null is not an object (evaluating 'this.canvas.getHighlighter(t).style')
showHighlighter (grapes.min.js:2:263826)
updateToolsLocal (grapes.min.js:2:266937)
(anonymous function) (grapes.min.js:2:260549)
forEach
onHovered (grapes.min.js:2:260438)
m (grapes.min.js:2:22592)
v (grapes.min.js:2:22263)
h (grapes.min.js:2:20216)
(anonymous function) (grapes.min.js:2:22156)
set (grapes.min.js:2:24026)
setHovered (grapes.min.js:11:128341)
handleHover (grapes.min.js:2:351496)
I (grapes.min.js:2:9008)
(anonymous function) (grapes.min.js:2:9208)
(anonymous function) (grapes.min.js:2:2321)
t (grapes.min.js:2:80874)

When I trigger getView().reset() the new elements in the DOM don't exist and their width and height are 0. I want to reset the view and then to trigger JS which will build the carousel.

@abozhinov
Copy link
Author

The same result you can get when trigger render instead of reset.

@abozhinov
Copy link
Author

@artf if you try on the demo is the same. Just drag the carousel and click on the slide.
Screenshot 2020-03-16 at 14 58 23

@abozhinov
Copy link
Author

abozhinov commented Mar 16, 2020

This is my temporary fix:

this.PageBuilder.Commands.extend('select-comp', {
            onHovered(em, component) {
                let result = {};

                if (component) {
                    component.views.forEach(view => {
                        const el = view.el;
                        const pos = this.getElementPos(el);

                        result = { el, pos, component, view };

                        this.updateToolsLocal(result);

                        if (el.ownerDocument === this.currentDoc) {
                            this.elHovered = result;
                        }
                    });
                }
            },
            hideHighlighter(view) {
                const el = this.canvas.getHighlighter(view);
                if (el) {
                    el.style.opacity = 0;
                }
            },
            showHighlighter(view) {
                const el = this.canvas.getHighlighter(view);
                if (el) {
                    el.style.opacity = '';
                }
            },
            getElementPos(el) {
                let data = this.canvas.getCanvasView().getElementPos(el);

                if (data.width === 0) {
                    const element = this.editor.Canvas.getDocument().getElementById(el.id);
                    if (element) {
                        data = element.getBoundingClientRect();
                    }
                }

                return data;
            },
        });

@artf
Copy link
Member

artf commented Mar 19, 2020

Thanks for the report @abozhinov I've seen this error somewhere in my logger but was never able to reproduce it.
First of all, I can't see any issue (or console logs) from your demo link (by following your steps) but I actually see the wrong toolbar position of the slider in the official demo, but still, no errors in the console.
Are you able to create another demo without using custom components (less nested components possible).

Obviously doing the check in the showHighlighter function skips the error but there is a hard assumption that this.canvas.getHighlighter(view) always exists, so I'd like to understand better the real problem

@artf artf added the bug label Mar 19, 2020
@abozhinov
Copy link
Author

@artf I don't why there are no errors in codepen. Second paramter in console log is NULL.
Screenshot 2020-03-21 at 21 29 57

@artf
Copy link
Member

artf commented Apr 3, 2020

I think we would need more help to understand better what is really happening and in this period I'm quite busy with other non-GrapesJS-related stuff 😞

@artf
Copy link
Member

artf commented Apr 7, 2020

Probably I've found the issue, seems to be a bug with components containing scripts. Should be fixed in the next release

@artf artf added this to To do in Release v0.16.12 via automation Apr 7, 2020
@abozhinov
Copy link
Author

abozhinov commented Apr 7, 2020

When overwrite these two methods in 'select-comp' command everything works perfectly.
The problem was that "el" doesn't exist in the DOM.

`

        onHovered(em, component) {
            let result = {};

            if (component) {
                const { view } = component;
                const { el } = view;
                const pos = this.getElementPos(el);

                result = { el, pos, component, view };

                if (pos.width > 0) {
                    this.updateToolsLocal(result);

                    if (el.ownerDocument === this.currentDoc) {
                        this.elHovered = result;
                    }
                }
            }
        },
        onSelect() {
            const { em } = this;
            const component = em.getSelected();

            if (component) {
                const { view } = component;
                const { el } = view;

                if (el) {
                    const pos = this.getElementPos(el);
                    const result = { el, pos, component, view };

                    if (pos.width > 0) {
                        this.elSelected = result;
                        this.updateToolsGlobal();
                        // This will hide some elements from the select component
                        this.updateToolsLocal(result);
                    }
                }
            } else {
                this.toggleToolsEl();
                this.lastSelected = 0;
            }
        }

@artf
Copy link
Member

artf commented Apr 23, 2020

@artf artf closed this as completed Apr 23, 2020
Release v0.16.12 automation moved this from To do to Done Apr 23, 2020
@abozhinov
Copy link
Author

I have the same error when select.

Screenshot 2020-04-23 at 10 06 14

@artf
Copy link
Member

artf commented Apr 28, 2020

I can't reproduce the toolbar issue anymore in the official demo. Without a reproducible demo, I can't do much

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

2 participants