changcheng / wro4j

Automatically exported from code.google.com/p/wro4j
0 stars 0 forks source link

Css url rewriting doesn't compute replace properly font rule with multiple url's #480

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?

Given this CSS: (for fonts, I know it is hacky, but well... )

@font-face {
    font-family: 'Trade Gothic Condensed Bold';
    src: url('../fonts/tradegothic-boldcondtwenty-webfont.eot');
    src: url('../fonts/tradegothic-boldcondtwenty-webfont.eot?#iefix') format('embedded-opentype'),
         url('../fonts/tradegothic-boldcondtwenty-webfont.woff') format('woff'),
         url('../fonts/tradegothic-boldcondtwenty-webfont.ttf') format('truetype'),
         url('../fonts/tradegothic-boldcondtwenty-webfont.svg#tradegothic-boldcondtwenty-webfont') format('svg');
    font-weight: normal;
    font-style: normal;
}

What is the expected output? What do you see instead?

Expected Output (with minimize=true) 1.46 - Correct 

@font-face {
    font-family: 'Trade Gothic Condensed Bold';
    src: url('../../s/af/fonts/tradegothic-boldcondtwenty-webfont.eot');
    src: url('../../s/af/fonts/tradegothic-boldcondtwenty-webfont.eot?#iefix') format('embedded-opentype'),
         url('../../s/af/fonts/tradegothic-boldcondtwenty-webfont.woff') format('woff'),
         url('../../s/af/fonts/tradegothic-boldcondtwenty-webfont.ttf') format('truetype'),
         url('../../s/af/fonts/tradegothic-boldcondtwenty-webfont.svg#tradegothic-boldcondtwenty-webfont') format('svg');
    font-weight: normal;
    font-style: normal;
}

Wrong Output (with minimize=true) 1.47 - Incorrect

@font-face {
    font-family: 'Trade Gothic Condensed Bold';
    src: url('../../s/af/fonts/tradegothic-boldcondtwenty-webfont.eot');
    src: url('../../s/af/fonts/tradegothic-boldcondtwenty-webfont.eot?#iefix') format('embedded-opentype'),
         url('../fonts/tradegothic-boldcondtwenty-webfont.woff') format('woff'),
         url('../fonts/tradegothic-boldcondtwenty-webfont.ttf') format('truetype'),
         url('../fonts/tradegothic-boldcondtwenty-webfont.svg#tradegothic-boldcondtwenty-webfont') format('svg');
    font-weight: normal;
    font-style: normal;
}

It only replaces the first occurrence of url( .* ) after the src.

What version of the product are you using? On what operating system?
MacOS / Windows7 / Weblogic 10.3.5 / Spring / Maven

Please provide any additional information below.

Original issue reported on code.google.com by dancalli...@gmail.com on 4 Jul 2012 at 5:03

GoogleCodeExporter commented 9 years ago
Btw, I am temporarirly switching back to 1.4.5 because on 1.4.6 I am hitting  
the issue described here  http://code.google.com/p/wro4j/issues/detail?id=434 
(mixing css and js) .

Also 1.46 and 1.47 meant 1.4.6 and 1.4.7 respectively

Original comment by dancalli...@gmail.com on 4 Jul 2012 at 5:10

GoogleCodeExporter commented 9 years ago
Actually the cssUrlRewriting processor works with 1.4.7, but doesn't produce 
expected outcome for this particular situation. 

I didn't know that it is possible to provide multiple url's for the same src in 
fonts... Do you know if there is a documentation about this? I would like to 
update the processor to respect the standard way of specifying url's.

After fixing the problem, you'll be able to switch back to 1.4.7 with patched 
version of CssUrlRewritingProcessor.

Original comment by alex.obj...@gmail.com on 5 Jul 2012 at 6:42

GoogleCodeExporter commented 9 years ago

Original comment by alex.obj...@gmail.com on 5 Jul 2012 at 6:43

GoogleCodeExporter commented 9 years ago
W3C CSS3 SPEC
http://www.w3.org/TR/css3-fonts/#font-face-rule

4.3 Font reference: the src descriptor

Name:   src
Value:  [ <uri> [format(<string> [, <string>]*)] | <font-face-name> ] [, <uri> 
[format(<string> [, <string>]*)] | <font-face-name> ]*
Initial:    N/A

Example from that same section
/* load WOFF font if possible, otherwise use OpenType font */
@font-face {
  font-family: bodytext;
  src: url(ideal-sans-serif.woff) format("woff"),
       url(basic-sans-serif.ttf) format("opentype");
}

From Mozilla font-face -experimental-spec:
https://developer.mozilla.org/en/CSS/@font-face/

Syntax

@font-face {
  font-family: <a-remote-font-name>;
  src: <source> [,<source>]*;
  [font-weight: <weight>];
  [font-style: <style>];
}

It is worth noting that there has been some discussion trying to find a cross 
browser declaration that works on all browsers and performs optimally.

Here is compatibility table of supported font formats:
http://webfonts.info/browser-support-overview

Paul Irish's Bulletproof font-face implementation
http://paulirish.com/2009/bulletproof-font-face-implementation-syntax/

A recap over Paul Irish's Bulletproof method
http://www.fontspring.com/blog/the-new-bulletproof-font-face-syntax/

And here is one example of the font declaration generated by one 
generator/converter that is often used.

Fontsquirrel

/* Generated by Font Squirrel (http://www.fontsquirrel.com) on June 21, 2012 */

@font-face {
    font-family: 'CustomGothic2009Regular';
    src: url('custom_gothic_2009-webfont.eot');
    src: url('custom_gothic_2009-webfont.eot?#iefix') format('embedded-opentype'),
         url('custom_gothic_2009-webfont.woff') format('woff'),
         url('custom_gothic_2009-webfont.ttf') format('truetype'),
         url('custom_gothic_2009-webfont.svg#CustomGothic2009Regular') format('svg');
    font-weight: normal;
    font-style: normal;

}

Original comment by dancalli...@gmail.com on 5 Jul 2012 at 2:26

GoogleCodeExporter commented 9 years ago
And yes, the original title I gave to the bug wasn't accurate. As you said, it 
worked, but in this particular case it wasn't generating the expected output.

Original comment by dancalli...@gmail.com on 5 Jul 2012 at 2:29

GoogleCodeExporter commented 9 years ago
Great, thanks for the links. 
The CssUrlRewritingProcessor uses a regex to capture all urls which needs to be 
replaced. Here it is:

cssUrlRewrite=(?is)([\w-]*\s*?:[^{]*?\b(?:src\b\s*=\s*['"](.*?)['"].*?|url\b\s*\
(['"]?(.*?)['"]?\)).*?)(?=(?:[\s|\r|\n]*?[\w-]*\s*:|}))

It is getting more and more complicated.... In order to fix this issue, this 
regex should be updated.

Original comment by alex.obj...@gmail.com on 5 Jul 2012 at 2:29

GoogleCodeExporter commented 9 years ago
Hey sure!, np, I'm glad to contribute, WRO4J has been really useful in my 
current project. Thanks for it!

Original comment by dancalli...@gmail.com on 5 Jul 2012 at 2:35

GoogleCodeExporter commented 9 years ago
Contributors are always welcome :).

There are plenty to issues to work on (code or documentation). If you are 
willing to contribute, feel free to fork the project from github and work on 
any of the existing issues ;)

Cheers,
Alex

Original comment by alex.obj...@gmail.com on 5 Jul 2012 at 2:37

GoogleCodeExporter commented 9 years ago

Original comment by alex.obj...@gmail.com on 1 Aug 2012 at 5:02

GoogleCodeExporter commented 9 years ago
Perhaps the processor should do several runs over the file, 
first replacing src\b\s*=\s*['"](.*?)['"].*?, 
then url\b\s*\(\s*['"]?(.*?)['"]?\s*\)

Aren't this the only part we care about?
As far as I understand, after staring at this expression for some ten minutes, 
the other parts of the regex is not needed.

Original comment by marvin.l...@gmail.com on 23 Nov 2012 at 3:59

GoogleCodeExporter commented 9 years ago
probably you are right.you could change it and see if all tests pass

Original comment by alex.obj...@gmail.com on 23 Nov 2012 at 4:59

GoogleCodeExporter commented 9 years ago
https://github.com/alexo/wro4j/pull/77

Created a version which cared only about the content in src='urltoreplace' and 
url(urltoreplace).
FallbackCssDataUriProcessor made it a bit more complex, so made it so this 
processor uses the old pattern.

Original comment by marvin.l...@gmail.com on 30 Nov 2012 at 12:17

GoogleCodeExporter commented 9 years ago

Original comment by alex.obj...@gmail.com on 30 Nov 2012 at 9:20

GoogleCodeExporter commented 9 years ago
Fixed in branch 1.6.x

Original comment by alex.obj...@gmail.com on 1 Dec 2012 at 11:00