CSS reformat problem

[BeckyDTP]

Hello,

I know you are preparing version 0.9.9, but maybe you can take a look at it.

One line CSS file with media query. Reformat CSS and a strange result.

Here you can clearly see it.

[dougmassay]

I see it. Weird. It's not a feature I use much, so I've never noticed. We'll look into it. Unfortunately, 0.9.9 cannot be held up to accommodate it. Any fix for this will be in a later release.

I will see if a plugin to reformat css (including media queries) can be slapped together as a workaround in the mean-time.

Thanks.

[kevinhendricks]

Yes we will look into this bug for the next release after 0.9.9. Thank you for your bug report.

Kevin

On Dec 22, 2017, at 5:24 PM, Becky notifications@github.com wrote:

Hello,

I know you are preparing version 0.9.9, but maybe you can take a look at it.

One line CSS file with media query. Reformat CSS and a strange result.

Here you can clearly see it.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[kevinhendricks]

Sigil/src/Misc/CSSInfo.cpp does not appear to grok media queries at all. We really should replace this CSS parser class with calls to a real CSS parser library built either in python, C, or C++ as CSS has moved on since this parser was designed and written.

We do have a CSS parser in the plugin library, so using python to fix this with python3lib calls probably makes themost sense unless we can find a good opensource CSS parser lib to pull into Sigil.

[wrCisco]

I just stumbled upon MyCSS, C css parser of the Modest engine, which looks promising (there is also a python binding of the engine). Remembering issue #219 I could try to do some testing and patch it into my version of Sigil, but I can't guarantee about timing...

[kevinhendricks]

Sounds good to me. I took a peek at it and I think that library relies on other my* component libraries so it is not quite standalone. We could theoretically extract and combine the parts we need into one css library if need be.

[kevinhendricks]

Whoops hit the close by mistake.

[wrCisco]

Yes, the up to date version of mycss is inside the Modest repository and seems to be dependent upon myhtml, myencoding and mycore (oh, and myport too). For now I built the entire engine and I'm doing some experiment just to wrap my head around its API...

[kevinhendricks]

How did this work out? I am thinkng if it looks good at all that adding this css parser to Sigil is orobably a good way to go.

[kevinhendricks]

This bug has been reported again privately via pm on Mobileread and really needs to get fixed for the next release. We can not have Prettying and Reformating CSS making a mess of things due to media queries.

The problem is that Sigil's current CSS parser was written long before media queries were thought of. The Sigil css parsing code itself is just too simplistic. I have examined Modest (mycss) and a few other C++ or C based CSS parsers (libcss, etc) but most of these have too many other outside requirements that would make adding them to Sigil a bit of an issue. Modest is small and builds out of the box on Linux and OSX (and hopefully Windows) but there is very little documentation on using it or the interface itself. It would also mean adding in yet another buffer, string allocation set of C code to deal with, have buffer overruns with, make mistakes with, etc.

@wrCisco, Since your (wrCisco)'s unused css removal plugin has found and worked around a few bugs in python's cssutils package and pretty much does what we need. It just seems logical to use python/cssutils as a solution for Sigil's outdated css parsing code permanently (or until a C++ or C library that is simple and stands alone can be found).

So before the next release of Sigil we will look into adapting wrCiscos's cssutils plugin code to work internally in Sigil.

This is just a head's up in how we plan to address this and related issues unless someone has a better idea.

If so, please let me know.

Thanks

[kevinhendricks]

Based on some recently provided test cases, the latest version of cssutils will not properly parse @media rules that use device-aspect-ratio or aspect-ratio as its tokenizer barfs on the "/" that is used to identify the ratio. Also cssutils does not recognize @supports as a @ keyword.

To make it orse, there is a bug report about aspect-ratios filed by Kovid almost 2 years ago with nothing being done to date.

So we may need to explore other options for a css parser or pull in our own copy of cssutils like we do with bs4.

Ideas and thoughts welcome on the best way forward.

[wrCisco]

Hi, sorry for the late reply. Months ago, I did some testing with mycss, but it didn't seem production ready, at least in regard to the serializer. If you want to work with my plugin, you're welcome (the version here on github is slightly more updated than the one on mobileread), but as you noted, there are some unresolved issues with cssutils. Did you see tinycss2? I'm not sure that's right for Sigil's needs, but seems a lot simpler to patch than cssutils if needed. If you need help with testing and/or patching, I'll try not to disappear as in the last few months...

[kevinhendricks]

I checked out tinycss2 but it is such a low level parser, we would basically have to write our own second stage parser. I looked and really could not find mch else. That said, knowing Kovid must have run into these same issues, I took a look at Kovid's internal calibre version of tinycss (check out the calibre source on github) and he has added support for properly parsing media queries in his version of tinycss. He also added a C based low level tokenizer to speed up his tinycss version.

So I am going to play arond with cssutils and try to add a <ratio> regex recognizer to the prodction code, and then add a Ratio class alongside the Dimension class to value.py and then extend the list of types for mediaqueries to include it but I have no idea if that is the right approach or not. The cssutils code is hard to follow as it keeps old code around and uses hierarchies of regex expressions to do its tokenizing andeven higher classes. As far as I can tell, further cssutils development is nonexistent and the project is close to dead.

If that fails, I will use Kovid's version of tinycss from his calibre tree and see how far I get with it. If it looks usable then we will clone it into our own tree if needed as tinycss is BSD and Kovid's additions to it are GPL.

If you get a free moment, would you take ua look at Kovid's version of tinycss directly from the calibre tree and play around with it and let me know what you think? I will post any cssutils code I can get to work in case you want totry it (assuming I can get something to work at all).

Thanks,

KevinH

[kevinhendricks]

Okay, here is a very very tentative patch for supporting aspect-ratio values in media queries in cssutils:

First I grabbed the latest cssutils-1.0.2.tar.gz and unpacked it. Then ran "2to3" on both the cssutils and encutils, to get it to function under python3.

Then I made the following changes:

--- cssproductions.py.keep  2018-12-14 13:10:32.000000000 -0500
+++ cssproductions.py   2018-12-14 11:58:40.000000000 -0500
@@ -74,6 +74,7 @@

     ('S', r'{s}+'), # 1st in list of general productions
     ('URI', r'{U}{R}{L}\({w}({string}|{url}*){w}\)'),
+    ('RATIO', r'{w}[0-9]+{w}\/{w}[0-9]+'),
     ('UNICODE-RANGE', r'{U}\+[0-9A-Fa-f?]{1,6}(\-[0-9A-Fa-f]{1,6})?'),
     ('IDENT', r'{ident}'),
     ('FUNCTION', r'{ident}\('),
--- prodparser.py.keep  2018-12-14 13:15:40.000000000 -0500
+++ prodparser.py   2018-12-14 13:16:26.000000000 -0500
@@ -801,6 +801,16 @@
                     )

     @staticmethod
+    def ratio(stop=False, nextSor=False):
+        "positive integer / positive integer"
+        return Prod(name='ratio',
+                    match=lambda t, v: t == PreDef.types.RATIO,
+                    toSeq=lambda t, tokens: (t[0], t[1].lower()),
+                    stop=stop,
+                    nextSor=nextSor
+                    )
+
+    @staticmethod
     def variable(toSeq=None, stop=False, nextSor=False, toStore=None):
         return Prod(name='variable',
                     match=lambda t, v: 'var(' == cssutils.helper.normalize(v),
--- css/value.py.keep   2018-12-14 13:00:31.000000000 -0500
+++ css/value.py    2018-12-14 13:03:57.000000000 -0500
@@ -107,7 +107,7 @@
                 [ NUMBER S* | PERCENTAGE S* | LENGTH S* | EMS S* | EXS S* |
                   ANGLE S* | TIME S* | FREQ S* ]
               | STRING S* | IDENT S* | URI S* | hexcolor | function
-              | UNICODE-RANGE S*
+              | RATIO S* | UNICODE-RANGE S*
               ;
             function
               : FUNCTION S* expr ')' S*
@@ -264,6 +264,7 @@
         prods = Choice(PreDef.hexcolor(stop=True),
                        PreDef.ident(stop=True),
                        PreDef.string(stop=True),
+                       PreDef.ratio(stop=True),
                        PreDef.unicode_range(stop=True),
                        )
         ok, seq, store, unused = ProdParser().parse(cssText, 'Value', prods)

which defined a new value type called ratio in cssproductions, added it to the PreDef class in value.py so it could be identified, and added it to prodparser.py

When I run the simple testcase provided by Kovid's bug report:

python3 -c "import cssutils; cssutils.parseString('@media (aspect-ratio:1280/800) { body { color: red }}')"

I now get no errors, instead of:

ERROR MediaQuery: Missing token for production expression END: ('CHAR', u'/', 1, 26) ERROR MediaList: No match: ('NUMBER', u'800', 1, 27) ERROR MediaQuery: No content.

This extends the Value class to recognize and allow ratios. Perhaps I should have kept the old Value class and instead created a new extended value class that would only be allowed in media queries. Who knows?

I have no idea if ratios can be used in any other place. The basic css parse/syntax docs do not really specify things in media queries well at all or really any at rule well at all. That is why tinycss2 went to the simpler approach but ...

Please give these changes a try in cssutils and if they work, I will work on the @supports rule next.

Then we can make all of our changes in our own copy of cssutils and absorb it into our tree or move to Kovid's tinycss version.

[dougmassay]

Would the plan be to move cssutils from the bundled python environment to the same place our patched bs4 module resides? If so, we'll have to be careful if we change the name like we did with sigil_bs4. There's some plugins that use the bundled cssutils. A name change would require them to update their plugins. Not that big of a deal, but if it could be avoided...

/plugin_launcher/python (the home of sigil_bs4) is already prepended to the existing sys.path, so there shouldn't be any confusion with installed system versions of cssutils if we don't change the name (thinking of Linux and externally configured pythons primarily, here).

Or were you thinking of using the patched version strictly for Sigil's own embedded python routines and leaving the standard cssutils in the plugin environment?

[kevinhendricks]

We could go either way. One solution might be to include it but keep the exact name and remove it from the bundled python list. That way our version will be found first I think (all it will have is bug fixes over the older official version). I do not plan on making any changes to the official interface methods, just fix bugs that do not seem to get fixed upstream.

[dougmassay]

That sounds like the best plan to me (keep the same name and remove it from the bundled python).

I'll see if I can dig out some meda query-laden css and test how cssutils will parse it. I certainly won't be the definitive voice of whether or not it's all working "correctly," though. Complex css really isn't my strong suit. ;)

[kevinhendricks]

FWIW, I now have an alternative cssutils patch that adds ratio a a PredDef but that never augments the Value class and that instead only extends the media features query expression by allowing PreDef.ratio().

Using this approach, aspect-ratio and device-aspect ratio would only be properly parsed inside media query expressions for media features. This would greatly lessen any impact of the change I am proposing.

I have no idea if device-aspect-ratio and aspect-ratio can be used anyplace other that a media query. I will have to look into that.

[wrCisco]

Are the four you posted the only changes you applied to cssutils? I tested it (both on the cssutils wheel already for python3 and the tarball) and I keep getting the same error as Kovid on the testcase.

Another place where there can be ratios in CSS syntax is inside the function calc (but that has its own production functions in cssutils). AFAIK the aspect-ratio is only used in media queries.

[kevinhendricks]

I probably missed something. I will check.

Sent from my iPhone

On Dec 14, 2018, at 7:33 PM, Francesco Martini notifications@github.com wrote:

Are the four you posted the only changes you applied to cssutils? I tested it (both on the cssutils wheel already for python3 and the tarball) and I keep getting the same error as Kovid on the testcase.

Another place where there can be ratios in CSS syntax is inside the function calc (but that has its own production functions in cssutils). AFAIK the aspect-ratio is only used in media queries.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

[kevinhendricks]

Checked and I did not miss any. These changes are enough to fix the issue with Kovid's testcase. Only those 3 files were changed. My guess is you are finding another cssutils somewhere in your python path. I am testing with a fresh python3 installation with no cssutils installed except for the version local to my testcase.

Here is an even simpler patch created by a recursive diff that should work too that restricts the ratio to being inside media queries.

changes2.patch.zip

[kevinhendricks]

Here is my exact work sequence with a python3 interpreter with no cssutils installed at all:

tar -zxvf cssutils-1.0.2.tar.gz unzip changes2.patch.zip 2to3 --output-dir=cssutils-v3/cssutils -W -n cssutils-1.0.2/src/cssutils 2to3 --output-dir=cssutils-v3/encutils -W -n cssutils-1.0.2/src/encutils cd cssutils-v3 patch -p0 < ../changes2.patch python3 -c "import cssutils; cssutils.parseString('@media (aspect-ratio:1280/800) { body { color: red }}')"

[kovidgoyal]

I think the best way forward is to fork cssutils and maintain ti ourselves. It's been on my TODO list to do that anyway, but I just haven't found the time. Currently I a basically use a combination of cssutils and tinycss to get around the bugs in cssutils.

[kovidgoyal]

I suggest setting up an organization in github and making the fork under that. Kevin and I can be maintainers of it (and anyone else who wants to volunteer). I do think we should rename it to something other than cssutils though, maybe python-css, to avoid confusion/stepping on the original project's toes.

[kovidgoyal]

As for sigil's plugin cssutils issues, that can be resolved by adding an importhook to the python environment to map cssutils imports to the new namespace.

[dougmassay]

As for sigil's plugin cssutils issues, that can be resolved by adding an importhook to the python environment to map cssutils imports to the new namespace.

You're right, of course. Actually, from within our launcher.py we can do something as simple as:

import whatever_we_call_the_new_cssutils sys.modules['cssutils'] = whatever_we_call_the_new_cssutils

The same could be done for our patched sigil_bs4 so that plugins could import using either the actual name, or the new namespaced one with the same results.

Thanks! I'll let you guys get back to cssutils forking/patching.

[kevinhendricks]

I am happy with the idea of forking cssutils and having a joint repo to maintain it. Also the "python-css" name sounds fine to me as long as it does not already exist in pypi or a google search. I could also see a "cssutils2" name if anyone were interested. I will probably post my patch on the official cssutils site bug report to be nice but I have little hope for any kind of response given he ignored your earlier serialize.py fix for years now.

I see work being done in calibre towards python3 compatibility. If so, we maintain our own fork of bs4 in python3 because of bugs and special uses, just in case you want to add that as jointly maintained as well.

[kovidgoyal]

I replaced beautifulsoup with the much faster, more accurate, HTML 5 parser for python I wrote a while ago -- https://github.com/kovidgoyal/html5-parser (this is based on gumbo and actually includes many of the fixes you have in your gumbo fork).

BeautifulSoup still exists in calibre, mainly for API compatibility reasons, actual HTML parsing is done by html5-parser, which builds a BS tree. So I think I'll pass on bs4, thanks.

Let's agree on a name and we can move forward. Candidates are:

css-parser, python-css, cssutils2, css_utils

this is in my preference ordering for the names. As far as I can tell, none of them is taken on pypi.

[kevinhendricks]

Wow! I had no idea you had forked gumbo as well. Your version of gumbo has fixes and speed-ups Sigil will need to pull in based on just a quick glance. As for bs4, since it is just a soup-like wrapper around another parser (ie a treebuilder like interface) for html5lib, lxml, or gumbo, we fixed a few things to make our bs4 serialize a bit better, and provide it for our plugin interface so that people who are used to soup can use a parser more easily in their plugins.

Since official gumbo support seems to have been abandoned, would you consider adding your version of gumbo to the proposed shared repo? I can then sync our version too it and help maintain that part.

As for a name for our cssutils fork, your "css-parser" choice would go well with your existing "html5-parser" name, so that works for me.

[wrCisco]

@kevinhendricks With your new changes to cssutilis all works smoothly, both with the 2to3 passage on the tarball and with the pip-installed version (I keep getting an error with the old patch, not sure what it is). It works without a problem with my plugin in Sigil, also.

As for the the joint repo with the css-parser, that's great news! I'm going to help as much as my capabilities can.

[kovidgoyal]

On Sun, Dec 16, 2018 at 07:56:26AM -0800, Kevin Hendricks wrote:

Wow! I had no idea you had forked gumbo as well. Your version of gumbo has fixes and speed-ups Sigil will need to pull in based on just a quick glance. As for bs4, since it is just a soup-like wrapper around another parser (ie a treebuilder like interface) for html5lib, lxml, or gumbo, we fixed a few things to make our bs4 serialize a bit better, and provide it for our plugin interface so that people who are used to soup can use a parser more easily in their plugins.

Sadly, bs4 is API incompatible with bs3, which means I cant use it in calibre.

Since official gumbo support seems to have been abandoned, would you consider adding your version of gumbo to the proposed shared repo? I can then sync our version too it and help maintain that part.

Sure, I have no objections.

As for a name for our cssutils fork, your "css-parser" choice would go well with your existing "html5-parser" name, so that works for me.

I have created an organization and repo for this and sent you an invite to become a fellow admin.

[kovidgoyal]

https://github.com/orgs/ebook-utils

is the new organization

[kovidgoyal]

feel free to upload the fork of cssutils with your patch

[kevinhendricks]

Thanks! I assume you still need python 2.7 support, but I really do not like needing/using 2to3 to generate separate versions of each file. Instead, I would like to move to one code-base based on python3 and then add the necessary pieces to make it work out of the box on both python2.7 and python3.x. This is similar to how we did the KindleUnpack project and our own Sigil plugin project as well. It should be relatively easy. Is that approach okay with you or do you want to stick with the current use of 2to3 during the build?

[kevinhendricks]

@wrCisco Glad to hear it now works! And I am happy to have you on board to help. Thanks

[kovidgoyal]

I hate 2to3 as well. I much prefer polyglot code -- i.e. code that runs on both. The approach I usually use is to run 2to3 in report mode and then just change everything it points out to work on both via simple polyglot module, such as polyglot.py in the calibre source code.

[kevinhendricks]

Yes your polyglot code is very much like our compatibility_utils.py code inside Sigil and KindleUnpack. It is amazing how much time and duplication of effort has occurred to fix the same problems to make one code base that works for both. I will take a shot at getting cssutils into some "polyglot" shape so that it runs on python2.7 and python3.x and then make the initial commit.

[kovidgoyal]

Great, let me know if you need a hand.

[kevinhendricks]

okay I have got one codebase for cssutils and encutils that seems to work on both python2 and 3 but I still need to convert the unittests in the cssutils/tests folder to run on both platforms as well so we can verify I haven't broken anything, especially for encoding detection.

I will try to get to that tomorrow and then start to build the repo.

[kevinhendricks]

@kovidgoyal Do we have a minimum python 2.x version we need to support? ie. 2.6 or 2.7 or earlier? The reason I ask is the use of next(tokens) vs tokens.next() was backported to python2.6 but no earlier, so I need to special case it in the code if we want to support anything earlier than python 2.6 but nothing would be needed if we use next(tokens) for python 2.6, 2.7, and 3.x

[kovidgoyal]

I am fine with >= 2.7.9

[kevinhendricks]

This will still take a while since I had to fix a few bugs to get all of the unittests to pass under python3.x. Having trouble getting the python2.x to do the same due to changes in code used in tests/basetest.py due to underlying differences in how os.path.abspath determines its current working directory from the python module that tries to load a file.

Perhaps, I will just plunge ahead tomorrow and take my best shot at making all of the test cases polyglot and then post what I have so others can see if they can figure out what is up.

[kovidgoyal]

Yeah go ahead and upload it, I will take a look this weekend.

[kevinhendricks]

Okay I pushed it. The repo is:

https://github.com/ebook-utils/css-parser

The code should now be in dual PY2, PY3 mode. I have not converted the test cases yet as I wanted to be able to verify correct behaviour with python 2.7 and python 3.x before merging the test code. So inside src/cssutils you will find a tests folder (py3) and a tests2 folder (py2).

To run the tests, simply copy the dotestsPY*.sh to the src directory after checkout and run it or any specific test form it.

Using Python3.X, it now passes all tests except for test_util.py where the test itself is flawed as it looks for http:/example.com/test.css but that file does NOT exist.

Using Python2.x, it fails the same test_util.py test for the same reason. It also fails one other test which is test_cssutilsimport.py as I get a TypeError: Item in "from list" not a string which is caused by this line (line 4) of that test file: from cssutils import * I think this is a sys.path issue but I am not sure .... It passes all other tests with python2.7.

Instead of using a polyglot module, I made what few changes were needed local to each file to make each self-contained and easy to understand when viewed alone or playing round with in isolation.

I have already included an improved version of the changes2.patch to fix the aspect-ratio issue. I have NOT added any other fixes we might have (the serialize fix and extending the media query types for Amazon, etc...

I have invited Sigil's co-maintainer @dougmassay to be an admin, and made sure @kovidgoyal was there as well.

@wrCisco if you want to work on this, just let me know and I will add you to the team with write privs.

@BeckyDTP please let me know your level of interest in working on this project as well.

@kovidgoyal what would be the best way to make your version of sigilgumbo/gumbo part of ebook-utils?

[kovidgoyal]

On Thu, Dec 20, 2018 at 08:46:47AM -0800, Kevin Hendricks wrote:

Using Python2.x, it fails the same test_util.py test for the same reason. It also fails one other test which is test_cssutilsimport.py as I get a TypeError: Item in "from list" not a string which is caused by this line (line 4) of that test file: from cssutils import * I think this is a sys.path issue but I am not sure .... It passes all other tests with python2.7.

I'll take care of fixing the tests.

Instead of using a polyglot module, I made what few changes were needed local to each file to make each self-contained and easy to understand when viewed alone or playing round with in isolation.

I have already included an improved version of the changes2.patch to fix the aspect-ratio issue. I have NOT added any other fixes we might have (the serialize fix and extending the media query types for Amazon, etc...

We can do that after I have fixed the tests.

@kovidgoyal what would be the best way to make your version of sigilgumbo/gumbo part of ebook-utils?

Not sure, the obvious way is to either move sigil-gumbo/gumbo to ebook-utils/gumbo or for me to create ebook-utils/gumbo with a copy of the gumbo code from html5-parser.

The problem is that html5-parser makes use of the fact that it is python based to automatically run an extensive html5 test suite including all the tests from html5lib. With pure gumbo, running those tests is not as easy/convenient.

[kevinhendricks]

okay, lets wait on gumbo. I think I grabbed all the bug fixes you made and can push them to Sigil. That just leaves your speedups but they can come later.

[kevinhendricks]

@kovidgoyal As for the one test failure in python2.X:

TypeError: Item in "from list" not a string

You may already know this but it seems that python2 will only accept str classes (bytes) not unicode for the __all__ list of classes to import. Obviously PY3 only accepts str (unicode).

So when we use from future import unicode_literals and then it is followed by an __all__ being defined, and use of: from cssutils import * it will of course fail.

This is used in test_cssutilsimport.py and possibly by other users at some point.

To fix this we could remove unicode_literals from future and redo the complete polyglot process and deal with u' all throughout the code, or convert the __all__ list properly for each platform after it is defined.

Something along the lines of the following would work.

__all__ = ['css', 'stylesheets', 'CSSParser', 'CSSSerializer']
__all__ = [str(n) for n in __all__]

I tried this approach on cssutils/__init__.py and now test_cssutilsimport.py now passes on python 2.x.

If we decide to go this approach, we would have to fix them all, which is easy enough to do.

That said, running the pep8 cleanup code, actually broke both PY2 and PY3 test_cssutilsimport.py tests because it moved all of the imports to the top of the file blindly. I committed fixes to put them back where they belong.

[kovidgoyal]

I actually think of imports as an anti-pattern and disallow it in codebases I maintain. However, from a backwards compatibility perspective, it might be worth preserving the ability to import on python 2.x -- personally I say let it break. Unless you need it for sigil plugins.

[kovidgoyal]

Oh also, I will be working extensively on this codebase for the next couple of days, so things are likely to break temporarily. I intend to move the tests out of the cssutils package, and setup CI on Travis and appveyor for testing instead. I am also going to rename the cssutils package to css_parser and probably move encutils into a subpackage.

[kevinhendricks]

Sigil moved a while back to Python 3 only for plugins, so letting it break is fine with me. It is easy to add back if it turns out to be needed later.

Thanks for letting me know about the breakage for the next week or so. We are in no rush. Please let us know when you think it is stable enough to start adding in things.

Thanks

[kovidgoyal]

You can now run the tests by using

python2 run_tests.py python3 run_tests.py

all tests pass.