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

View stringify utility should sort CSS classes and styles #4207

Closed
pomek opened this issue Nov 7, 2017 · 11 comments · Fixed by ckeditor/ckeditor5-engine#1184
Closed

View stringify utility should sort CSS classes and styles #4207

pomek opened this issue Nov 7, 2017 · 11 comments · Fixed by ckeditor/ckeditor5-engine#1184
Assignees
Labels
package:engine status:discussion type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@pomek
Copy link
Member

pomek commented Nov 7, 2017

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.

@pomek pomek self-assigned this Nov 7, 2017
@szymonkups
Copy link
Contributor

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.

@szymonkups szymonkups changed the title Attributes are sorted but classes and styles are skipped View stringify should sort CSS classes and styles Nov 7, 2017
@szymonkups szymonkups changed the title View stringify should sort CSS classes and styles View stringify utility should sort CSS classes and styles Nov 7, 2017
@szymonkups
Copy link
Contributor

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.

@pomek pomek removed their assignment Nov 13, 2017
@Reinmar
Copy link
Member

Reinmar commented Nov 14, 2017

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.

@wwalc
Copy link
Member

wwalc commented Nov 16, 2017

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

@Reinmar
Copy link
Member

Reinmar commented Nov 16, 2017

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.

@pomek
Copy link
Member Author

pomek commented Nov 16, 2017

So, can I continue work on it?

@pomek pomek self-assigned this Nov 16, 2017
@Reinmar
Copy link
Member

Reinmar commented Nov 16, 2017

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.

@pomek
Copy link
Member Author

pomek commented Nov 16, 2017

After introducing sorting the attributes/values, ~80 tests fail. Of course, we can fix these tests but I'm wondering whether we could use ckeditor/ckeditor5-core#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.

@Reinmar
Copy link
Member

Reinmar commented Nov 16, 2017

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

So which tests fail after your change?

@pomek
Copy link
Member Author

pomek commented Nov 16, 2017

A lot of in images and UI.

@szymonkups
Copy link
Contributor

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

szymonkups referenced this issue in ckeditor/ckeditor5-engine Nov 20, 2017
Fix: View stringify utility now sorts CSS classes and values in `style` attribute. Closes #1179.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 14 milestone Oct 9, 2019
@mlewand mlewand added status:discussion type:bug This issue reports a buggy (incorrect) behavior. package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine status:discussion type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants