[BUG] CSS parser invalidly parses certain duplicate declarations

[tommedema]

Tested on latest dev branch commit 39be1e0. This problem occurred on one of my templates, so it's a real world issue. It took me about half a day to create this minified test case, so I hope it helps.

If you have a template with the following CSS:

<div id="gjs">
      <style>
        body {
          font-family: Helvetica Neue;
        }
        body, span {
          font: inherit;
        }
        body {
          font-family: Helvetica Neue;
        }
      </style>
      <span>Find</span>
</div>

A browser renders this correctly as such:

JSFiddle: https://jsfiddle.net/szLp8h4n/192/

But grapes renders it incorrectly:

JSFiddle: https://jsfiddle.net/szLp8h4n/194/

@artf this is another example where I would recommend not creating your own parser (though you call it a "traverser"), and simply rely on the browser's parser. E.g. it should not be necessary to create an internal cache of all components and styles, when the browser itself already has a DOM tree and all styling information etc can be retrieved on-demand once a component is selected. However, it would be too big of a change for grapes to do this for this particular issue. So in this case I would recommend investigating the cause of and fixing this particular case.

Do you have an idea what might cause this? If you don't have time to fix it I would like to fix it myself because this is a pressing issue on my side. But I would very much appreciate some hints what would get me started.

Thanks a ton <3

[artf]

when the browser itself already has a DOM tree and all styling information etc can be retrieved on-demand once a component is selected

Probably this is something I've tried to build before grapesjs, and to be honest, after few weeks of work I realized that the result was just a big, unmaintainable, unreliable, SLOW, piece of crap. Believe me Tom, keeping the state of such a big application inside the DOM, it's just not an option. And again, I'm using the browser's parser but the problem is just how the CSS state of rules is build.

BTW, GrapesJS before creating a new style rule checks if one, with same properties, already exists. If one is found, it updates its style, so this is what happens:

1. The first body declaration is created and inserted inside the state:

[
	// First rule
	{
		selectors: [], // <-- this one is used only for Selectors (classes)
		selectorsAdd: 'body', // Additonal selectors
		style: {...},
	}
]

2. Same for the second one

[
	// First rule
	{
		selectors: [],
		selectorsAdd: 'body',
		style: {...},
	}, 
	// Second rule
	{
		selectors: [],
		selectorsAdd: 'body, span', // different from the previous
		style: {...},
	}
]

3. The next body declaration, is the same as the first one, therefore only its style will be updated

[
	// First/Third rule
	{
		selectors: [],
		selectorsAdd: 'body',
		style: {...}, // style updated
	}, 
	// Second rule
	{
		selectors: [],
		selectorsAdd: 'body, span',
		style: {...},
	}
]

So as you see the problem here is that the third rule is inserted inside the first one and this breaks the cascading principle.
Probably, the quick & dirty solution would be to add some option which ignores all the checks and adds all rules inconditionally.

[tommedema]

@artf it's surprising to me that this is slower because I thought the browser would be heavily optimized for this, but you definitely have more experience with this project so I'll trust in your judgement ;)

I'm definitely in favor of an option to just add all rules unconditionally. This makes sense to me because for me the browser is truth and there should be no need to do additional checking on top of what the browser tells you.

Would you like to do this or should I create a PR? If the latter, could you tell me where these checks are done? Is it really in this getter that you previously linked? I would expect the checks to be done outside of a getter.

[artf]

I'd really appreciate a PR.
I think the check would be ok inside the add method