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

Crashes on font shorthand with custom properties #26

Closed
tornqvist opened this issue May 21, 2019 · 8 comments
Closed

Crashes on font shorthand with custom properties #26

tornqvist opened this issue May 21, 2019 · 8 comments
Labels
bug Something isn't working

Comments

@tornqvist
Copy link

A recent fix (8e27afd) for short hand font properties will throw an error if one is using custom properties.

E.g. this rule causes the regex in said commit to not match.

.foo {
  font: var(--font-style) var(--font-weight) 1em/var(--line-height) var(--font-family);
}
@tornqvist tornqvist changed the title font shorthand with custom properties crash Crashes on font shorthand with custom properties May 21, 2019
@leeoniya
Copy link
Owner

thanks for the report, will take a look later today.

on a more general note, i wonder if it makes sense to resolve css vars. my initial thought is that it would open a big can of worms beyond the simple static example. also, was DropCSS incorrectly removing font-faces for you prior to this commit, and/or did you have them whitelisted?

@leeoniya
Copy link
Owner

leeoniya commented May 21, 2019

ok, i've added custom props resolving in a993e63. seems to do okay in this test:

<!doctype html>
<html>
  <head>
    <script src="../dist/dropcss.js"></script>
    <script>
      function run() {
        let clean = dropcss({
          html: `
            <html><body></body></html>
          `,
          css: `
            :root {
              --font-style: italic;
              --font-weight: bold;
              --line-height: 1.6em;
              --font-family: 'Open Sans';
            }

            @font-face {
              font-family: "Open Sans";
              src: url("/fonts/OpenSans-Regular-webfont.woff2") format("woff2"),
                   url("/fonts/OpenSans-Regular-webfont.woff") format("woff");
            }

            body {
              font: var(--font-style) var(--font-weight) 1em/var(--line-height) var(--font-family);
            }
          `,
        });

        console.log(clean.css);
      }
    </script>
  </head>
  <body>
    <button onclick="run()">run()</button>
  </body>
</html>

let me know how this works out.

@tornqvist
Copy link
Author

Sorry, my example was a simplified version of my actual use case. We use custom properties for both the font shorthand and the individual properties. This is more in line with what I'm working with and it's unfortunately still crashing with this fix:

:root {
  --font-style: italic;
  --font-weight: bold;
  --line-height: 1.6em;
  --font-family: 'Open Sans';
  --font: var(--font-style) var(--font-weight) 1em/var(--line-height) var(--font-family);
}

@font-face {
  font-family: "Open Sans";
  src: url("/fonts/OpenSans-Regular-webfont.woff2") format("woff2"),
        url("/fonts/OpenSans-Regular-webfont.woff") format("woff");
}

body {
  font: var(--font);
}

@leeoniya
Copy link
Owner

making me really work for this one 🤣

@tornqvist
Copy link
Author

Hey, no pressure. I appreciate you taking your time! I think I overlooked your first comment and in response to that; no it did not remove my font-faces.

I'll try and have a look and see if I can make sense of this.

@leeoniya
Copy link
Owner

so, c3869cc implements a comprehensive custom props resolver, which properly handles multi-level composites as your example uses.

the issue that now exists is with the fact that i've been using the index/offset for detecting whether eg font-family: is within a @font-face declaration or being used as a property. since flattening all var(--*) defs changes the previously found offsets, i need to re-work how this disambiguation works. another issue that surfaced is that the regex's think that --font-family: blah is a "used" instance similar to font-family: blah.

I think I overlooked your first comment and in response to that; no it did not remove my font-faces.

i think the reason it did not remove it is because your custom prop is named --font-family:, so it ran into the issue mentioned above and considered it "used" :) if DropCSS properly ignored custom prop defs, it would have removed it.

so there's some more re-work left until it's fully resolved.

@leeoniya
Copy link
Owner

@tornqvist try the current build. this should now be handled correctly.

@tornqvist
Copy link
Author

Works like a charm. Thank you sooo much! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants