Ckeditor5: View stringify utility should sort CSS classes and styles

Created on 7 Nov 2017  路  11Comments  路  Source: ckeditor/ckeditor5

One of test on Edge fails because of order of the styles don't match to defined in test:

expected 
'<a style="overflow:visible;font-weight:bold;"><b style="color:red;display:block;"></b></a>' 
to equal 
'<a style="font-weight:bold;overflow:visible;"><b style="color:red;display:block;"></b></a>'

We could sort everything.

engine discussion bug

All 11 comments

View utilities are sorting attribute names:
https://github.com/ckeditor/ckeditor5-engine/blob/07a01b1acc8a51cb8c6107e7644ba342a3268bdc/src/dev-utils/view.js#L827
But classes and styles are not sorted, this will help to have consistent output of normalizeHtml method from utils.

I was talking about this with @oskarwrobel and @f1ames and have some doubts. Styles and classes order have a meaning in terms of CSS, so sorting the output might not be a good idea. Maybe instead of that, we should check why the output is different and fix issue there.

Styles and classes order have a meaning in terms of CSS, so sorting the output might not be a good idea.

But we want to do this only for tests. And I don't know any case where it would matter for real. Actually, I'm surprised that the order may matter.

A nice summary that confirms what @szymonkups wrote and that explains cases when the order of CSS classes matters: https://stackoverflow.com/a/15672815/524419

OK, so it applies to [class="class1 class2"] :D That's not something we do.

Anyway, we're talking about a util we use in tests. So we don't have to be worried about such scenarios. Actually, I would say that CKEditor doesn't need to be worried about them at all.

So, can I continue work on it?

I think so. If the problem we're going to solve here is running tests in multiple browsers then sorting attrs in test utils will be a perfect helper.

After introducing sorting the attributes/values, ~80 tests fail. Of course, we can fix these tests but I'm wondering whether we could use https://github.com/ckeditor/ckeditor5-core/pull/108 and accept two versions of HTML (first for Edge, second for other browsers). It means this ticket will be invalid.

We have only one test which fails because of the order values in [style] attribute.

We have only one test which fails because of the order values in [style] attribute.

So which tests fail after your change?

A lot of in images and UI.

Let's fix those ~80 tests. It will make them more stable and safe for the future.

Was this page helpful?
0 / 5 - 0 ratings