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

Google fonts are imported, break HTTPs when protocol relative #632

Closed
LoicMahieu opened this issue Jul 30, 2015 · 10 comments
Closed

Google fonts are imported, break HTTPs when protocol relative #632

LoicMahieu opened this issue Jul 30, 2015 · 10 comments
Labels
Milestone

Comments

@LoicMahieu
Copy link

Google fonts @import are resolved. Anyway it works when imported. The problem is, you can not keep URLs as protocol relative.

Consider:

echo "@import url(//fonts.googleapis.com/css?family=Lato:400,700,400italic);" | cleancss

which output:

@font-face {
    font-family: Lato;
    font-style: normal;
    font-weight: 400;
    src: local('Lato Regular'), local('Lato-Regular'), url(http://fonts.gstatic.com/s/lato/v11/v0SdcGFAl2aezM9Vq_aFTQ.ttf) format('truetype')
}
@font-face {
    font-family: Lato;
    font-style: normal;
    font-weight: 700;
    src: local('Lato Bold'), local('Lato-Bold'), url(http://fonts.gstatic.com/s/lato/v11/DvlFBScY1r-FMtZSYIYoYw.ttf) format('truetype')
}
@font-face {
    font-family: Lato;
    font-style: italic;
    font-weight: 400;
    src: local('Lato Italic'), local('Lato-Italic'), url(http://fonts.gstatic.com/s/lato/v11/LqowQDslGv4DmUBAfWa2Vw.ttf) format('truetype')
}

You notice that src url is in HTTP. It obviously break application when hosted in production, under HTTPs.

My question:

  • Is there a way to blacklist some URL in order to not import them ?
    It will keep my url with protocol relative.

Thanks for response and thanks to contributors for this so useful module!

@jakubpawlowicz
Copy link
Collaborator

That's a good point indeed. The only way to stop it (so far) is to use --skip-import switch.

@LoicMahieu
Copy link
Author

My workaround for now is to set HTTPs for loading font. So on font-face contains https link and it does not break production code.
Obviously I don't think that a good idea to resolve Google font import. Location of the files may changes.

Possible solution:

  • Add option for skipping remote import. It will disable fetching of remote @import and keep it intact. ?
  • Add option for defining exclusion mask ?

By searching in the repository I found https:/petetak/clean-css/blob/698efdc1a387706ebfb36178677e539280e8abc4/test/unit-test.js#L770 . I don't find theses tests in the current code base. I am new to this project.

Thanks

@jakubpawlowicz
Copy link
Collaborator

I'll think of the best approach here, it could be even a combination of those.

Re that unit test, it's from an old version of clean-css when there was no remote importing at all, so all such URLs were kept intact.

Stay tuned for updates!

@jakubpawlowicz jakubpawlowicz added this to the 3.4 milestone Aug 16, 2015
jakubpawlowicz added a commit that referenced this issue Aug 19, 2015
This refers to #632, but just confirms we do the right thing.
jakubpawlowicz added a commit that referenced this issue Aug 19, 2015
With `processRemoteImport: false` / `--skip-remote-import` one can
entirely skip remote `import` processing.
@jakubpawlowicz
Copy link
Collaborator

I found out the reason you see URLs changed from //fonts.gstatic... into http://fonts.gstatic... is due to Google rewriting them on the fly, e.g. curl "http://fonts.googleapis.com/css?family=Lato:400,700,400italic"

I added two features you suggested on a branch https:/jakubpawlowicz/clean-css/tree/632-remote-import which will likely make to master. The syntax is as follows:

API: new CleanCSS({ processRemoteImport: ['!fonts.googleapis.com'] })
CLI: cleancss --skip-remote-import fonts.googleapis.com

To skip all remote imports:

API: new CleanCSS({ processRemoteImport: false })
CLI: cleancss --skip-remote-import all

Does it makes sense?
Any feedback is more than welcome!

@LoicMahieu
Copy link
Author

Thanks for your work!

processRemoteImport will fix the issue! But I was thinking that it can be expand to all @import processing. It makes sense to skip import, regardless it is remote or not.
We can reuse the option processImport and use it as an array like processRemoteImport. An array is truely and so it will maybe not involve lot of modification.
Anyway processRemoteImport: false make sense too for allowing user to globally disable remote import.

API:

  new CleanCSS({
    processImport: [
      '!fonts.googleapis.com',
      '!./some/local/file.css'
    ]
  })

@jakubpawlowicz
Copy link
Collaborator

I think you have a fair point. How about adding some special keywords, like !local or !remote?

jakubpawlowicz added a commit that referenced this issue Aug 24, 2015
With `processRemoteImport: false` / `--skip-remote-import` one can
entirely skip remote `import` processing.
@jakubpawlowicz
Copy link
Collaborator

@LoicMahieu what's more, we'd need to go with an extra option, e.g. --skip-import-from / processImportFrom: ... as we can't overload CLI's --skip-import without a major version bump, something I am reluctant to do. This is a commander.js limitation which does not allow list arguments to be optional.

I would keep processImport as is and add an extra one now, and unify these two in the next major version.

jakubpawlowicz added a commit that referenced this issue Aug 25, 2015
Adds an option to skip importing of specific URLs.

So far we only had an option to skip inlining of all imports, but
with this commit a fine-grained control is added, e.g. import from
all but fonts.googleapis.com:

API: `new CleanCSS({ processImportFrom: ['!fonts.googleapis.com'] })`
CLI: `cleancss --skip-import-from fonts.googleapis.com`

To skip all local imports:

API: `new CleanCSS({ processImportFrom: ['!local'] })`
CLI: `cleancss --skip-import-from local`

To skip certain local and all remote imports:

API: `new CleanCSS({ processImportFrom: ['!path/to/file', '!remote'] })`
CLI: `cleancss --skip-import-from path/to/file,remote`
jakubpawlowicz added a commit that referenced this issue Aug 25, 2015
So far we only had an option to skip inlining of all imports, but
with this commit a fine-grained control is added, e.g. import from
all but fonts.googleapis.com:

API: `new CleanCSS({ processImportFrom: ['!fonts.googleapis.com'] })`
CLI: `cleancss --skip-import-from fonts.googleapis.com`

To skip all local imports:

API: `new CleanCSS({ processImportFrom: ['!local'] })`
CLI: `cleancss --skip-import-from local`

To skip certain local and all remote imports:

API: `new CleanCSS({ processImportFrom: ['!path/to/file', '!remote'] })`
CLI: `cleancss --skip-import-from path/to/file,remote`
jakubpawlowicz added a commit that referenced this issue Aug 25, 2015
So far we only had an option to skip inlining of all imports, but
with this commit a fine-grained control is added, e.g. import from
all but fonts.googleapis.com:

API: `new CleanCSS({ processImportFrom: ['!fonts.googleapis.com'] })`
CLI: `cleancss --skip-import-from fonts.googleapis.com`

To skip all local imports:

API: `new CleanCSS({ processImportFrom: ['remote'] })`
CLI: `cleancss --skip-import-from local`

To skip certain local and all remote imports:

API: `new CleanCSS({ processImportFrom: ['local', '!path/to/file'] })`
CLI: `cleancss --skip-import-from remote,path/to/file`
@jakubpawlowicz
Copy link
Collaborator

I have just merged this change to master. The API is as you suggested with the exception of processImportFrom vs processImport option.

@jakubpawlowicz
Copy link
Collaborator

It's just been released with clean-css 3.4. 🎆

Thanks @LoicMahieu for all valuable input!

@LoicMahieu
Copy link
Author

Awesome ! Thanks @jakubpawlowicz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants