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

Special character sequences are interpreted instead of copied as is #26

Closed
agabrys opened this Issue Jan 23, 2018 · 15 comments

Comments

2 participants
@agabrys
Member

agabrys commented Jan 23, 2018

Special character sequences are interpreted instead of copied, for example this selector:

.v-ie .base .v-inline-datefield-button:after {
  content: "\200B";
}

turns into this after spliting:

.v-ie .base .v-inline-datefield-button:after {
  content:"​"
}

@agabrys agabrys self-assigned this Jan 23, 2018

@agabrys agabrys added the bug label Jan 23, 2018

@agabrys agabrys added this to the 2.0.0 milestone Jan 23, 2018

@agabrys

This comment has been minimized.

Member

agabrys commented Jan 23, 2018

This problem is caused by CSS Parser, see: #85 Character escape sequences gets corrupted by the parser

@agabrys

This comment has been minimized.

Member

agabrys commented Jan 23, 2018

The fix is ready. I'm waiting for the CSS Parser 0.9.25 release.

agabrys added a commit that referenced this issue Jan 24, 2018

@agabrys agabrys changed the title from Special character sequences are interpreted instead of copied to Special character sequences are interpreted instead of copied as is Jan 24, 2018

Smasherr added a commit to Smasherr/css-splitter-maven-plugin that referenced this issue Jan 27, 2018

Smasherr added a commit to Smasherr/css-splitter-maven-plugin that referenced this issue Jan 27, 2018

@agabrys

This comment has been minimized.

Member

agabrys commented Jan 27, 2018

@Smasherr I see that you added CssFormat in more places. I'll do the same thing.

Smasherr added a commit to Smasherr/css-splitter-maven-plugin that referenced this issue Jan 27, 2018

@Smasherr

This comment has been minimized.

Smasherr commented Jan 27, 2018

@agabrys I figured out that StylePropertyConverter is crucial to modify. I'll create a PR so you can have a look at the all changes at once.

@Smasherr

This comment has been minimized.

Smasherr commented Jan 27, 2018

@agabrys I'm not sure though, if you want this behavior as default or maybe introduce this option in your plugin and delegate it to the parser. But that's something to start with ;-)

@Smasherr

This comment has been minimized.

Smasherr commented Jan 27, 2018

Ah, you don't need my PR because you already implemented it. I just oversaw it.

@agabrys

This comment has been minimized.

Member

agabrys commented Jan 27, 2018

@Smasherr I fixed StylePropertyConverter in feature/#26 branch. You modified code located in develop branch. That is the reason why you had to fix StylePropertyConverter on your own. I see that you use CSSFormat in more classes. I'm verifying where the change is needed (e.g. CharsetRuleConverter does not require any changes, because CSSCharsetRuleImpl#getCssText(CssFormat) does not use the format parameter).

@Smasherr

This comment has been minimized.

Smasherr commented Jan 27, 2018

I'm verifying where the change is needed

Well I was sure you'll verify it 👍

Another tiny difference is that I cast to CSSFormatable instead of CSSValueImpl in StylePropertyConverter, which is maybe more safe? Do we maybe need an instanceof check there?

@agabrys

This comment has been minimized.

Member

agabrys commented Jan 27, 2018

I checked and we also should use CSSFormat in:

  • ImportRuleConverter
  • MediaRuleConverter
  • UnknownRuleConverter
  • UnsupportedRuleException

I'll push new code next 30 minutes.

agabrys added a commit that referenced this issue Jan 27, 2018

#26 use CSSFormat for import, media and unknown rules; refactor Style…
…PropertyConverter; use junitparams in more tests
@agabrys

This comment has been minimized.

Member

agabrys commented Jan 27, 2018

I pushed a final solution. If you have any free time - please check the code. Tomorrow I'll merge it to develop and next release a new version: 2.0.0.

@Smasherr

This comment has been minimized.

Smasherr commented Jan 28, 2018

@agabrys Looks good to me, nice work!

@agabrys

This comment has been minimized.

Member

agabrys commented Jan 28, 2018

@Smasherr cool, thanks 👍 I'll release it today's evening.

@Smasherr

This comment has been minimized.

Smasherr commented Jan 28, 2018

@agabrys This is great, because I can remove our workaround for #23 and #26 tomorrow at the office and see how our stylesheets look with the new plugin version. I will give you my feedback once again after that.

agabrys added a commit that referenced this issue Jan 28, 2018

#26 Special character sequences are interpreted instead of copied as is
- upgrade CSS Parser to 0.9.25
- use CSSFormat for import, media and unknown rules
- refactor StylePropertyConverter (use CssFormatter)
- add JUnitParams library to remove loops from tests
- add @font-face verification to compatibility tests

@agabrys agabrys closed this Jan 28, 2018

@Smasherr

This comment has been minimized.

Smasherr commented Jan 30, 2018

@agabrys I tested the fixes and can say the plugin works like a charm! I compared our original css with the splitted and saw that both important properties as well as escaped strings are still in place. I noticed some minor differences, but I don't know if they are worth opening an issue for them:

  • #fff is converted to rgb(255, 255, 255)
  • .base [draggable=true] is converted to .base [draggable="true"]
  • line-height: 25px;/**blablabla**/ is converted to line-height: 25px;

They are not critical so you decide if you want to have them fixed ;-)

@agabrys

This comment has been minimized.

Member

agabrys commented Jan 30, 2018

Thank you for verification 👍

#fff is converted to rgb(255, 255, 255)

I have only two options provided by the CSS Parser, print always:

  1. hex code with 6 letters: #fff#ffffff and rgb(255, 255, 255)#ffffff
  2. rgb function: #fffrgb(255, 255, 255)

I think that #ffffff is more concise and readable than rgb(255, 255, 255) so a new ticket for it is a good idea - I created #28 but be aware that it has very low priority 😉

.base [draggable=true] is converted to .base [draggable="true"]

I cannot fix it without some changes in CSS Parser. Luckily those two attribute selectors are equivalent, so the change won't break anything. I think there is a very low chance that CSS Parser will be changed in this area (they have much more important issues to fix).

line-height: 25px;/**blablabla**/ is converted to line-height: 25px;

CSS Parser removes all comments. To change this behavior CSS Parser has to be rewritten or I have to find another parser. Let's say it is impossible right now 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment