paolochiodi / htmlcompressor

A work in progress port of google's htmlcompressor.jar
Other
150 stars 26 forks source link

remove_intertag_spaces default is inconsistent between Rack and standard object #4

Closed mcoms closed 11 years ago

mcoms commented 11 years ago

In https://github.com/paolochiodi/htmlcompressor/blob/master/lib/htmlcompressor/compressor.rb, :remove_intertag_spaces => false but in https://github.com/paolochiodi/htmlcompressor/blob/master/lib/htmlcompressor/rack.rb, :remove_intertag_spaces => true.

This creates an inconsistency which isn't expected, nor explained in the readme.

mcoms commented 11 years ago

Intertag space removing also behaves incorrectly with link tags. See this issue: https://github.com/middleman/middleman-minify-html/issues/7

paolochiodi commented 11 years ago

thank you for reporting. I'll investigate the behavior of the original google's lib and if the case I'll make the README more clear

paolochiodi commented 11 years ago

Hi, the default options for the htmlcompressors class came directly from the orginal google html compressor and are fairly basics. While developing the rack middleware I used more aggressive default options for most common cases.

Removing the intertag spaces is considered "dangerous" because it can change the layout. Making this operation smarter (avoiding strip of spaces around inline tags) isn't an easy one nor it is safer (html compressor doesn't have any knowledge of css that could make a block element inline) and as since is not planned for the near future.

Please note that even disabling this option contiguous whitespaces are collapsed into a single space as per html spec (this operation is, indeed loseless).

Anyway I've just pushed a clearer version of the README and disabled remove_intertag_spaces in the default options to make them safer. I hope this will be even clearer in the future since I've planned to extract the rack middleware in his own gem and to add some documentation for the options.

Thank you

ps: due to problems with rubygems I'll upload the fixed gem later on