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

[Question/Bug] grapesjs throws a DOMException if you import HTML contents with invalid (numbered) attributes #2029

Closed
arachnosoft opened this issue May 23, 2019 · 7 comments

Comments

@arachnosoft
Copy link

arachnosoft commented May 23, 2019

Hi @artf ,

We got a report that when you import some HTML code with incorrect attributes, such as

<td class="cell" 01234="0" >Hello world! </td>

grapes.js throws the following Uncaught DOMException: Failed to execute 'setAttribute' on 'Element': '01234' is not a valid attribute name.
at HTMLTableCellElement. (https://grapesjs.com/js/grapes.min.js?v0.14.61:3:3229)
at x (https://grapesjs.com/js/grapes.min.js?v0.14.61:3:1245)
at m.each (https://grapesjs.com/js/grapes.min.js?v0.14.61:3:4121)
at m.attr (https://grapesjs.com/js/grapes.min.js?v0.14.61:3:3195)
at m.attr (https://grapesjs.com/js/grapes.min.js?v0.14.61:3:3276)
at i.updateAttributes (https://grapesjs.com/js/grapes.min.js?v0.14.61:2:60111)
at i.renderAttributes (https://grapesjs.com/js/grapes.min.js?v0.14.61:2:60919)
at i.render (https://grapesjs.com/js/grapes.min.js?v0.14.61:2:61020)

Numbered invalid attribute

Unless that's an explicit choice from you, do you think you could do something to catch this exception on the parser, or at least give us a way to handle it on our own? Like some option to explicitely ignore such attributes?

Sure, the user should not import such content, but we can't always ensure that they won't do anything like this ;)

Thanks for your help.

-Maxime

@ankx06
Copy link

ankx06 commented May 27, 2019

I have also faced this issue while setting an invalid attribute/style for image height/width.

Currently, I have handled using a try-catch block

  static updateInnerHTMLofComponent(component, newContent) {
    // Set inner content empty               // this is optional
    component.set("content", "");

    // Add new content in component. This will remove existing child components and create new child components
    try {
      component.components(newContent);
    } catch (error) {
      // Handling component render failed when user set invalid attributes to one of the component
      this.hendleComponentRenderFailed(component, error.message);
    }
  }

If the newContent has multiple components and only one component have the error, due to exception none of the components renders, at least the components which are not in error should render correctly and the component in error can render after ignoring invalid such attribute?

Thanks,
Ankit

@artf
Copy link
Member

artf commented May 29, 2019

The solution proposed by @ankx06 is actually correct. The best way to handle the exception is to use try-catch.

@artf artf closed this as completed May 29, 2019
@jmchaves
Copy link

@ankx06, In which event do I have to write the function updateInnerHTMLofComponent?

@ankx06
Copy link

ankx06 commented Aug 7, 2019

@jmchaves this is not called from any event. This is the function to update component's inner components/html.

@arachnosoft
Copy link
Author

arachnosoft commented Aug 14, 2019

Just to keep you informed, I applied the try/catch fix suggested by @ankx06 and @artf to our code, but in a generic way.

I wanted to ensure that the exception would be catched in all cases when setComponents() is being called.
Either from our code, or, more importantly (and not covered by @ankx06 's example, as far as I know) from the Import Code window.

So, I used a JavaScript closure technique to wrap the original setComponents() function within a new one, in which I put the try/catch: https://stackoverflow.com/a/10427757

var myGrapesJsObject = grapesjs.init(...);
myGrapesJsObject.setComponents = (function (originalFct) {
		  return function (components) {
			try {
				originalFct(components);
			}
			catch (ex) {
                                window.alert('Parse error: ' + ex);
				}
		  }
		})(myGrapesJsObject.setComponents);

And it works perfectly. No more blocking DOMExceptions.

Thank you all for your suggestions!

@DavidSporer
Copy link

@arachnosoft I tried this but it didn't help in my case. Is there a specific grapesjs version needed for this in order to work? It looks like the ideal solution to the problem (until there may be a fix in grapesjs itself).

So, I used a JavaScript closure technique to wrap the original setComponents() function within a new one, in which I put the try/catch: https://stackoverflow.com/a/10427757

var myGrapesJsObject = grapesjs.init(...);
myGrapesJsObject.setComponents = (function (originalFct) {
		  return function (components) {
			try {
				originalFct(components);
			}
			catch (ex) {
                                window.alert('Parse error: ' + ex);
				}
		  }
		})(myGrapesJsObject.setComponents);

@artf
Copy link
Member

artf commented Jun 16, 2020

In the next version, the error will be caught and the rendering of invalid Component definitions will be skipped

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants