Duplicate @media queries trigger an AssertionError

alexwlchan commented 7 years ago

I have the following HTML file with the same @media query appearing twice:

<!-- /tmp/example.html -->
<html>
  <head>
    <style>
      @media screen and (min-width: 600px) { p { display: none; } }
      @media screen and (min-width: 600px) { p { display: none; } }
    </style>
  </head>
  <body>
    <p>Hello world</p>
  </body>
</html>

If I run the following script:

from mincss.processor import Processor

p = Processor()
p.process('file:///tmp/example.html')

I get an AssertionError:

$ python testcase.py
Traceback (most recent call last):
  File "testcase.py", line 7, in <module>
    p.process('file:///tmp/example.html')
  File "/Users/chana/.virtualenvs/tempenv-24ba183892d9d/lib/python3.6/site-packages/mincss/processor.py", line 152, in process
    processed = self._process_content(content, self._bodies)
  File "/Users/chana/.virtualenvs/tempenv-24ba183892d9d/lib/python3.6/site-packages/mincss/processor.py", line 380, in _process_content
    assert old in content, old
AssertionError: @media screen and (min-width: 600px) { p { display: none; } }

The expected behaviour in this case would be:

If the HTML contains a <p> tag, the CSS is reduced to one instance of the @media query
If the HTML does not contain a <p> tag, both instances of the @media query are removed

I hit this from a weird corner case in my build system where it sometimes renders duplicate CSS selectors, and then I pass it into mincss for extra processing. I have a fairly easy workaround – this could be considered a case of “garbage in, garbage out” – but having tracked it down, I thought I might as well report it.

Version info:

$ python --version
Python 3.6.0

$ pip freeze
appdirs==1.4.0
cssselect==1.0.1
lxml==3.7.3
mincss==0.11.2
packaging==16.8
pyparsing==2.1.10
six==1.10.0

peterbe commented 7 years ago

Yeah, we need to fix this. I've seen it too. When I originally wrote mincss I didn't care and I hacked a solution. It's time to fix it. I think the implementation is kinda sucky and uses some really basic string replacements. I bet you can fix it. It shouldn't be hard to write a unit test that proves it's broken (using your example above) and then you can hack and hack until the unit test passes.

Do you know how to run the unit tests?

alexwlchan commented 7 years ago

I think the implementation is kinda sucky and uses some really basic string replacements. I bet you can fix it. It shouldn't be hard to write a unit test that proves it's broken (using your example above) and then you can hack and hack until the unit test passes.

Sounds good. I don’t have time right now, but I’ve stuck a note in my task list to come back to this if I get time.

Do you know how to run the unit tests?

Looking in .travis.yml suggests that nosetests is enough?

peterbe commented 7 years ago

Looking in .travis.yml suggests that nosetests is enough?

Yes. There might be some notes about it in the readme too.

peterbe / mincss

Duplicate @media queries trigger an AssertionError #46