Refactor specificity

[barryvdh]

Don't use a value, but compare the 3 values in a seperate class. And don't add the order to the specificity, only use it when the specificity is equal. This will prevent cases where the specificity is increased too much because many rules. This replaces #56

This is following the spec better, so that 2,0,1 > 1,12,1 (vs 201 < 221, which is incorrect)

/cc @stof What do you think, something like this compare also for the CssSelector component?

[tijsverkoyen]

I think this will be no longer an issue, as the class uses XPath instead of regexes

[barryvdh]

Yes it is, this has nothing to do with each other.
But if you don't like adding an extra class, I can submit a PR to at least seperates the specificity from the order.

[tijsverkoyen]

Oeps, my mistake, must have missed the point.
Can you provide me with an example HTML/CSS-file where it goes wrong.

[barryvdh]

I used the basic example from: http://zurb.com/ink/downloads/templates/sidebar-hero.html (source zip)

The specificity fails on Click me block, probably because you add the $i to each specificity, so when the first element is 1, the 100th is 101, while it only has a specificity of 1..

[barryvdh]

And for the change in Specificity, I don't have any example that goes wrong, but see this issue for a discussion: symfony/symfony#11400

You can't just compare specificity values like this, but it still works, as long as you don't have more then 10 classes/id's/parts. But comparing it like this should work no matter what.

[stof]

I tjhink this logic does not work properly for #id.class1.class2::hover for instance. And it also break for [data-type="foo bar"].

In the first case, it will set it to 1, 0, 0 instead of 1, 2, 1. In the second case, it will set it to 0, 0, 2 instead of 0, 1, 0

[barryvdh]

Indeed, but that was also the case earlier.
We could bring in the Selector Parse from the CssSelector component, if that's not a bit overkill?

[barryvdh]

But we can only use the compare method from the Specificity class when Symfony 2.6 is released, so we would have to use the value (shouldn't be too big a problem, but neither are perfect)

[stof]

I already thought about getting the specificity from the Symfony parser. I haven't yet tried how much refactoring it would involve in the library (it would not be doable when using CssSelector::toXPath static method as this one does not give you the parsed selector at all btw).

and bumping the dependency to 2.6+ would also be an issue, because it would make the library unusable in Symfony <2.6 projects. If we use the value, we would face the same bug when you have big selectors

[stof]

however, the first fix is indeed to separate the specificity and the order handling for the comparison, which is making the bug much more likely to happen.

[barryvdh]

Maybe counting the occurrences of the # and . in a string, as starters?

    foreach ($chunks as $chunk) {
        $first = substr($chunk, 0, 1);
        $a = substr_count($chunk, '#');
        $b = substr_count($chunk, '.');
        $c = ($first !== '.' && $first !== '#') ? 1 : 0;
        $specificity->increase($a,$b,$c);
    }

I'm not very good at regexes, otherwise that might be an idea.

[barryvdh]

And perhaps better just merge this PR and then create a new issue to focus on getting the actual correct specificity? What do you think @tijsverkoyen ?

[barryvdh]

I changed the calculation of the specificity to match that of the css_parser from premailer, which should have pretty good results, except for a few edge cases.
Results from http://www.w3.org/TR/selectors/#specificity are all correct, except the last on, using :not(), which is difficult because it counts the attributes inside the not() function, not the :not () function itself. (But that's probably very edge-case and probably isn't used that much)

Selector	CSS3 Spec	Old	New
*	0,0,0	1	0,0,0
li	0,0,1	1	0,0,1
ul li	0,0,2	2	0,0,2
ul ol+li	0,0,3	4	0,0,3
h1 + *[rel=up]	0,1,1	5	0,1,1
ul ol li.red	0,1,3	12	0,1,3
li.red.level	0,2,1	10	0,2,1
#x34y	1,0,0	100	1,0,0
#s12:not(FOO)	1,0,1	100	1,0,0

The 2 cases from @stof has the first one different then he said in his example, but I think the output from the regex is correct (the hover counts as b factor, not c).

Selector	CSS3 Spec	Old	New
#id.class1.class2::hover	1,3,0	100	1,3,0
[data-type="foo bar"]	0,1,1	2	0,1,1

[pbowyer]

Good patch, this sorted many problems with my emails built on Zurb Ink templates.

What it didn't sort was the following cascade:

    #hire-details td {
        width: auto !important;
        font-size: 13px;
        line-height: 16px;
    }
    #hire-details td.icon-col {
        width: 25px !important;
    }

When inlined, the CSS styles for <td class="icon-col"> have "width: auto !important" even with this patch, not the correct "width: 25px !important" as the specificity should be.

[barryvdh]

You are on version 1.4.1, right? Didn't this PR solve that? #58
Maybe that isn't 100% correct.

When the unit tests PR is merged, it would be easier to test for these edge cases and avoid problems..

[pbowyer]

Yes, I'm on 1.4.1 with #58 merged, and the above !important CSS has this error on that version. I have to ship this email today so have worked around it, but would be good to test for & fix in a future version.

[barryvdh]

Can you check with #62 ?