CSS selector failures from our test suite

[Frenzie]

Going through some of these quickly because at least some of these should fall in easy to fix territory. Ignoring XML namespaces which are unsupported and probably not very important to us.

tl;dr The recent improvements greatly reduced the previously disturbingly large failure rate. Fantastic work!

css3-modsel-15.epub [fixed]

css3-modsel-155d.epub

css3-modsel-156.epub [fixed]

css3-modsel-15b.epub [fixed]

css3-modsel-170d.epub [fixed]

I don't think :first-child is supported atm. If it is, all the test involving it (including much more basic ones) seem to be failing. ;-) Same for :first-line, ::first-letter and the whole family.

css3-modsel-175c.epub

css3-modsel-22.epub

css3-modsel-4 [fixed]

@poire-z Might be relatively easy to fix this one?

css3-modsel-43b, 44b [fixed]

@poire-z Hm, !important is complicated. :-P

css3-modsel-45 [fixed]

css3-modsel-86 [fixed]

css3-modsel-88 [fixed]

[poire-z]

You didn't tell how many work (%, nb?), so all that red..., mhhpf :)

I guess the only serious one is that #id seems not to be working.

Some are quite twisted and related to parsing (\. \13 &)

foo & adress, p (dunno what the first part means): aren't these 2 different selectors, and the p one is equivalent as being standalone?

#test#fail: should just be as if separated by , or space, or else ?

:first-child is not supported, but could be easy to add (as it involves nodes, like the others: parents, ancestor)
:first-line, ::first-letter would be harder, as it's only a part of a node, and would involve the renderer.
transparent is not know as a color, nor as a named value in the css code.

The last ones involve more than 2 rules (blockquote > div p): I think the current code only supports 2 (one in the selector itself (_id, the element it will apply to) and one in a SelectorRule, that can only match related to current element)

[Frenzie]

Most of 'em work, not counting stuff that isn't implemented at all. I didn't tally but I'm sure there's 30+ out of 65 that work. ;-) I don't really know why the test suite is an incomplete copy containing these specific files anyway.

Btw, if you don't mention the test identifiers it's really hard to know what we're talking about, so I'm not directly referring to any tests here but just to the selectors mentioned.

foo & adress, p (dunno what the first part means): aren't these 2 different selectors, and the p one is equivalent as being standalone?

Yes, and the point is that only the invalid selector should be dropped rather than the whole statement.

#test#fail: should just be as if separated by , or space, or else ?

It should be nothing. It's an invalid selector that should fail to match anything.

transparent is not know as a color, nor as a named value in the css code.

It's very old: https://www.w3.org/TR/CSS1/#background-color

[poire-z]

Yes, and the point is that only the invalid selector should be dropped rather than the whole statement.

css3-modsel-156.epub seems to expect the whole statement should be dropped. crengine does what you say.

#test#fail: should just be as if separated by , or space, or else ?
It should be nothing. It's an invalid selector that should fail to match anything.

css3-modsel-15b.epub seems to expect #pass#pass should match. crengine does what you say.

[Frenzie]

css3-modsel-156.epub seems to expect the whole statement should be dropped. crengine does what you say.

Then obviously I'm wrong, though for my own piece of mind mostly because I misinterpreted what you were saying. :-) I thought you were mentioning them because they went against what one would expect, so I thought "if the entire statement &invalid nonsense, p shouldn't be dropped, then naturally the valid selector p should still be applied instead." You misled me, or rather I misled myself. :-P

That being said, #pass#pass doesn't make much sense to me but I guess it's still valid CSS. #id.classname means an element with an ID and a classname, whereas the more common #id .classname means a descendant of #id with .classname. So if it's the same for IDs then #test#fail means an element with ID test and fail (which is impossible) whereas #pass#pass would mean with ID pass and pass, which is stupid but apparently nevertheless possible.

[Frenzie]

Okay, so as half expected it's not stupid per se, but not something we should care too much about:

https://www.w3.org/TR/CSS21/selector.html#id-selectors

If an element has multiple ID attributes, all of them must be treated as IDs for that element for the purposes of the ID selector. Such a situation could be reached using mixtures of xml:id [XMLID], DOM3 Core [DOM-LEVEL-3-CORE], XML DTDs [XML10] and namespace-specific knowledge.

[poire-z]

Should that match (another thing that should be case insensitive but is currently not) ?

DIV#toto { color: red; }
<div id="toto">i am toto</div>

This works as expected:

div#toto { color: red; }
<div id="toto">i am toto</div>

[poire-z]

The last ones involve more than 2 rules (blockquote > div p): I think the current code only supports 2

I was wrong, it seems to support more:

i ~ span > em a#tutu {color: lime;}
<i>som text</i>
<span> blah<em>blah<b> <a id="tutu" href="#toto">should be green</a><b></em></span>

I'll add ~ (i ~ span), because it's easy and will allow closing koreader/koreader#2845.
Also, + fails when there is a text node (even a single space) between the elements:

i + span > em a#tutu {color: lime;}
<i>som text</i><span> blah<em>blah<b> <a id="tutu" href="#toto">will be green</a><b></em></span>
<i>som text</i> <span> blah<em>blah<b> <a id="tutu" href="#toto">will NOT be green</a><b></em></span>

[Frenzie]

There's some mismatched tag stuff going on in your HTML there btw. The parsed DOM may be different than you were going for, although since you're not testing B probably not in a way that affects the purpose of the testcase.

Edit: d'oh, accidentally removed this:

Should that match (another thing that should be case insensitive but is currently not) ?

Should not in EPUB/XHTML (case sensitive), should in HTML.

[poire-z]

I guess + should ignore any text node when checking for "immediately preceeding"...

i + b { color: blue; }
<i>italic</i><b>no space</b><br/>
<i>italic</i> <b>space</b><br/>
<i>italic</i>blah<b>blah before</b><br/>
<i>italic</i><u>underline</u><b>element before</b><br/>

Firefox:

KOReader:

[poire-z]

Should be soon fixed:
css3-modsel-15.epub
css3-modsel-15b.epub
css3-modsel-4
css3-modsel-45

css3-modsel-86 and css3-modsel-88 looked like they should work, but I understand now the test (that includes Nondeterministic in their description...): the check could and should take multiple paths to find a match. The current code takes only one direct path.

[Frenzie]

Should be soon fixed:

Wow, I was really just documenting this for some value of later. :-)

I wonder if there's an easy way to unit test against regressions in this area. Something like an md5sum of the rendered bitmap, perhaps.

[poire-z]

Something like an md5sum of the rendered bitmap, perhaps.

Then everytime we fix something else that shifts a pixel, or update freetype, we'll have to remake screenshots to get new md5sum... No thanks :)

[Frenzie]

That particular example (known FT/CSS change) could probably be easily automated, but sure. Just brainstorming.

It looks like Mozilla uses something they call reftests.
https://developer.mozilla.org/en-US/docs/Mozilla/Creating_reftest-based_unit_tests

The principle is pretty simple. You have the actual unit test and a fake simpler page that simply sets the background to green without fancy selectors. Combine it with a couple of failsafes for the simple tests (e.g., this is what a green background looks like) and you've got yourself a testsuite that Just Works™ without that issue.

Maybe I'll set up some test scaffolding in base. Ideally it'd be here but for the moment I'm not sure if I see a sensible way of doing that. (Without putting in hours.)

[poire-z]

css3-modsel-43b, 44b with background-color: transparent will be fixed by just adding:

--- a/crengine/src/lvstsheet.cpp
+++ b/crengine/src/lvstsheet.cpp
@@ -687,6 +687,13 @@ bool parse_color_value( const char * & str, css_length_t & value )
             return true;
         }
     }
+    if ( substr_icompare( "transparent", str ) ) {
+        // Make it an invalid color, but a valid parsing so it
+        // can be inherited or flagged with !important
+        value.type = css_val_unspecified;
+        value.value = 0;
+        return true;
+    }
     return false;
 }

[Frenzie]

Oh, it's as simple as not supporting transparent? Yes, please add that. :-)