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

broken escaping #578

Open
eeve opened this Issue Jul 14, 2017 · 32 comments

Comments

Projects
None yet
@eeve
Copy link

eeve commented Jul 14, 2017

What is the current behavior?
The escape symbol \ was lost

.grid            { display: flex; flex-wrap: wrap; }
.grid.\-top      { align-items: flex-start; }
.grid.\-middle   { align-items: center; }
.grid.\-bottom   { align-items: flex-end; }

=>

.grid            { display: flex; flex-wrap: wrap; }\n.grid.-top      { align-items: flex-start; }\n.grid.-middle   { align-items: center; }\n.grid.-bottom   { align-items: flex-end; }\n

What is the expected behavior?
Keep the escape symbol \

https://github.com/eeve/test.v1

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Jul 14, 2017

@eeve btw, why your use \ in selectors?

@jschlieber

This comment has been minimized.

Copy link

jschlieber commented Jul 20, 2017

I'm running into the exact same issue. In my case escape sequences are used in Predix-UI CSS Modules, e.g:

https://github.com/PredixDev/px-spacing-design/blob/db32341b44fee68cace50d5d236306f768bbed26/_trumps.spacing.scss#L175-L181

So that basically means I can't use those u-m+ and u-m++ classes.

Any ideas for a quick workaround?

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Jul 20, 2017

@jschlieber unfortunately no, try to found and fix problem today

@jschlieber

This comment has been minimized.

Copy link

jschlieber commented Jul 26, 2017

@evilebottnawi Any news on this issue?

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Jul 26, 2017

@jschlieber I was on vacation, in the near future I will take care of this, thanks for waiting

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Jul 26, 2017

@jschlieber u-m+ and u-m++ are invalid css classes (https://jigsaw.w3.org/css-validator/#validate_by_input), your should use u-m\+ and u-m\+\+

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Jul 26, 2017

@jschlieber also .\-top {} and .-top {} are valid class selectors and mean the same thing.

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Jul 26, 2017

Bug inside postcss-modules-local-by-default and postcss-modules-scope, they remove \ character. But i wonder use case.

@jschlieber .u-m\+ and etc selectors work good (#584).

@TrySound what do your think about this?

@jschlieber

This comment has been minimized.

Copy link

jschlieber commented Jul 27, 2017

@evilebottnawi Ok, but what you're telling me is that i can't define my css selector as:

.u-m++{
  ...
}

and I'm totally fine with that. But still, if classes are defined like this

.u-m\00002b{
  ...
}

as in the PredixUi CSS library (which btw is valid css). I can't use it like it is intended, e.g.

<div class="u-m+">
  ...
</div>

because it ends up as

.u-m00002b{
  ...
}

so I would have to use it like this

<div class="u-m00002b">
  ...
</div>
@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Jul 27, 2017

@jschlieber don't understand your. Your can use .u-m\00002b in your css and css-loader works fine. (Test failed)
Test:

test("selector with unicode", ".u-m\\00002b { a: b c d; }", [
  [1, ".u-m\\00002b { a: b c d; }", ""]
]);

Input:

.u-m\00002b {
    border: 10px solid black;
}

Output:

.u-m\00002b {
  border: 10px solid black;
}

Just try to test in your project.
I agree what css-loader should not remove \, but it is happens only with selector where escaped is not necessary (example if your have \.-top {}, your can use .-top {}, because \.-top {} and .-top {} are mean the same thing).

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Jul 27, 2017

@jschlieber Thanks for issue, confirmed. Let's wait what says @TrySound (he has access for postcss-* plugins for css-loader)

@jschlieber

This comment has been minimized.

Copy link

jschlieber commented Aug 10, 2017

@evilebottnawi Thanks for your efforts. Any news on this one?

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Aug 10, 2017

@jschlieber no 😞
@TrySound friendly ping

@TrySound

This comment has been minimized.

Copy link
Collaborator

TrySound commented Aug 10, 2017

I guess it's something in selector tokenizer. Patch release will help. However I don't have access yet.

@TrySound

This comment has been minimized.

Copy link
Collaborator

TrySound commented Aug 10, 2017

@6palace

This comment has been minimized.

Copy link

6palace commented Jan 19, 2018

+1 from us as well

@RoopGuron

This comment has been minimized.

Copy link

RoopGuron commented Jan 19, 2018

+1 This fix is needed.

@EvanLovely

This comment has been minimized.

Copy link

EvanLovely commented Feb 7, 2018

I've got a workaround detailed here

@EvanLovely

This comment has been minimized.

Copy link

EvanLovely commented Feb 7, 2018

I found where this is being caused: ./lib/getLocalIndent.js has a .replace() function that replaces a big RegEx with - (if you change that character and run you can see it being used).

@EvanLovely

This comment has been minimized.

Copy link

EvanLovely commented Feb 7, 2018

It appears that by the time it hits this file, classes like .u-hide\@large are already u-hide@large, and then after that line become u-hide-large.

@EvanLovely

This comment has been minimized.

Copy link

EvanLovely commented Feb 7, 2018

Hmm... also turning CSS modules off with options.modules = false "fixes" it.

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Feb 15, 2018

Just infromation:

.icon-caret-left:before {
   content: '"\\f10c"'
}

Also broken

This was referenced Feb 15, 2018

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Feb 15, 2018

Also

body {
  font-family: '微软雅黑'; /* some chinese font name */
}
@martin-minovski

This comment has been minimized.

Copy link

martin-minovski commented Mar 24, 2018

+1

@themoch

This comment has been minimized.

Copy link

themoch commented Apr 9, 2018

I'm having a slightly different issue.

Loader

{
          loader: "css-loader",
          options: {
            sourceMap: true,
            camelCase: "dashes",
            importLoaders: 1,
            modules: true,
            localIdentName: "[name]__[local]___[hash:base64:5]"
          }
        },

input.css

.myStyle {
    content: '\e901';
}

transforms to:

output.css

styles__myStyle___1LKpi {
    content: '\E901';
}

☝️ notice the uppercasing of E. This breaks the unicode used in the font icon

@pitgrap

This comment has been minimized.

Copy link

pitgrap commented Oct 24, 2018

Another similar bug:

input.css

.glyphicon-search:before {
  content: "\e003 \fe0e";
}

output.css

.glyphicon-search:before {
  content: "\E003   \FE0E";
}
  1. Transform to uppercase like @themoch described
  2. Instead of one space there are now 3 spaces between the characters!
@jihb

This comment has been minimized.

Copy link

jihb commented Nov 7, 2018

Another similar bug:

input.css

.glyphicon-search:before {
  content: "\e003 \fe0e";
}

output.css

.glyphicon-search:before {
  content: "\E003   \FE0E";
}
  1. Transform to uppercase like @themoch described
  2. Instead of one space there are now 3 spaces between the characters!

same problem.

@jihb

This comment has been minimized.

Copy link

jihb commented Nov 7, 2018

Another similar bug:
input.css

.glyphicon-search:before {
  content: "\e003 \fe0e";
}

output.css

.glyphicon-search:before {
  content: "\E003   \FE0E";
}
  1. Transform to uppercase like @themoch described
  2. Instead of one space there are now 3 spaces between the characters!

same problem.

delete space fix this problem

@evilebottnawi evilebottnawi added this to the 2.0.0 milestone Dec 5, 2018

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Dec 5, 2018

All fixed in master exclude selectors, it is very hard, workaround use .u-m\+ instead .u-m\00002b

@evilebottnawi evilebottnawi modified the milestones: 2.0.0, 2.0.1 Dec 5, 2018

@evilebottnawi evilebottnawi changed the title The escape symbol "\" was lost broken escaping Dec 5, 2018

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Dec 5, 2018

Broken also (when modules enable):

:root {
  --title-align: center;
  --sr-only: {
    position: absolute;
    width: 1px;
    height: 1px;
    padding: 0;
    overflow: hidden;
    clip: rect(0,0,0,0);
    white-space: nowrap;
    clip-path: inset(50%);
    border: 0;
  };
}
@dulakm

This comment has been minimized.

Copy link

dulakm commented Jan 18, 2019

Hello,
please, is there any update on the issue? I can confirm another problem with escaping similar to the one mentioned in #752.

When using CSS modules and @custom-selector functionality, even the most basic example escaping goes wrong:

@custom-selector :--heading h1, h2, h3;
article :--heading + p {
    margin-top: 0;
}

=>

@custom-selector :--heading h1, h2, h3;
article :\--heading + p {
    margin-top: 0;
}
@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Jan 18, 2019

@dulakm problem in css modules plugins (for postcss), feel free investigate and send a PR

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