This repository has been archived by the owner. It is now read-only.

Cssfilter #477

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

listen2 commented Jul 10, 2012

Updated to the latest cssutils, remove duplicate code, and a few misc improvements.

Owner

spladug commented Jul 10, 2012

Is there a change to setup.py that's missing?

r2/r2/lib/cssfilter.py
@@ -31,8 +31,8 @@
from pylons.i18n import _
from mako import filters
-import os
-import tempfile
+#import os
@spladug

spladug Jul 10, 2012

Owner

Good find of the unused imports, but could you just delete the lines please?

r2/r2/lib/cssfilter.py
}
+cssutils.profile.addProfile("Reddit compat", custom_values, custom_macros)
@spladug

spladug Jul 10, 2012

Owner

Perhaps the profile name should be pulled out into a constant so it's not duplicated?

Nitpick: reddit should not be capitalized :)

@listen2

listen2 Jul 10, 2012

Contributor

Can you explain what you mean by duplication?

@spladug

spladug Jul 10, 2012

Owner

The string "Reddit compat" is written out twice in this code. If you wanted to change the name it'd require changing in two places.

r2/setup.py
@@ -82,7 +82,7 @@
"cython>=0.14",
"SQLAlchemy==0.7.4",
"BeautifulSoup",
- "cssutils==0.9.5.1",
+ "cssutils",
@spladug

spladug Jul 10, 2012

Owner

Looking at the changelog for cssutils, it looks like they break compatibility quite frequently. Would it be better to pin a specific (new) version?

Owner

spladug commented Jul 15, 2012

I'm running this new code on every stylesheet in the database currently and so far I've found a few regressions:

  • text-shadow does not appear to always work:
  • "black 0 0 10px" is not a valid value for CSS property "text-shadow"
  • "inherit" is not a valid value for CSS property "text-shadow"
  • Likewise for box-shadow:
    • "#0c0 0 0 10px" is not a valid value for CSS property "box-shadow"

I'll let you know if I come across more in my testing.

That said, this is already proving its worth as it's found several places where the subreddit stylesheets were flat out wrong! (some url(<a href="http://i.imgur.com/...">) type stuff and border: 1px solid 2px solid black), so this is very exciting. :)

Owner

spladug commented Jul 15, 2012

Additionally, and I'm not sure if this is new behaviour or not, I don't appear to get line numbers with my validation errors.

Contributor

listen2 commented Jul 16, 2012

None of the three examples above are valid CSS. See the grammars at W3C [1] [2] [3]. The legacy code seems to be letting them through due to a custom rule. I can add that rule if you want to preserve the incorrect behavior.

It looks like the new cssutils doesn't report line numbers for validation errors, only for parse errors. If it's important, I can look again more closely later this week.

[1] http://www.w3.org/TR/css3-text/#text-shadow
[2] http://www.w3.org/TR/css3-background/#box-shadow
[3] http://www.w3.org/TR/css3-background/#ltshadowgt

Owner

spladug commented Jul 16, 2012

Both inherit and color length length length seem to validate just fine for me on the W3C CSS validator. MDN says that's valid syntax, though the CSS3 spec seems to indicate otherwise (but perhaps I'm misreading the grammar there).

Contributor

listen2 commented Jul 16, 2012

Ah, my mistake; I misinterpreted the spec.

Owner

spladug commented Jul 16, 2012

Alrighty, looks like that covers everything in the existing stylesheets that is actually valid. Gonna do a final code-review pass then we should be good to go! :)

Contributor

alienth commented May 9, 2013

Unfortunately there is an infinite recursion bug in cssutils 0.9.10 holding this back. We'll follow up once that can be addressed.

cBlank commented May 10, 2013

Thanks a lot, can't wait. (Hopefully not another 10 months xD).

Contributor

empyrical commented Apr 10, 2014

If it's still present, how can the infinite recursion bug be reproduced?

Owner

spladug commented Apr 29, 2014

Closing because this has been obviated by the replacement of cssutils with tinycss2. Thanks for the contribution and sorry for it not getting merged.

@spladug spladug closed this Apr 29, 2014

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.