Bug #96413 for CSS-Inliner: some non-inline-able pseudo selectors not getting skipped

Thu Jun 12 21:55:24 2014 lxp [...] cpan.org - Ticket created

Given CSS with declarations like :hover, :focus and :active, calling the "inlinify" method results in a number of warnings being raised: Pseudoclass :hover not supported at .../HTML/Query.pm line 284. Pseudoclass :focus not supported at .../HTML/Query.pm line 284. Pseudoclass :last-of-type not supported at .../HTML/Query.pm line 284. Pseudoclass :nth-child not supported at .../HTML/Query.pm line 284. Pseudoclass :first-of-type not supported at .../HTML/Query.pm line 284. Pseudoclass :active not supported at .../HTML/Query.pm line 284. In my case, these are propagating into cron output and triggering emails every time I'm using this distribution. I realise that these errors are coming from HTML::Query rather than from CSS::Inliner directly, but since these features just aren't supported in inlined CSS, I wonder whether CSS::Inliner should somehow be responsible for preventing these warnings from occurring rather than HTML::Query. I'm happy to be told that this needs to be filed as an HTML::Query bug instead. My current workaround is to use Capture::Tiny to capture STDERR while the "inlinify" method runs, then use a regex to filter out pseudoclass- related warnings (and propagate any remaining warnings).

Thu Jun 12 21:56:03 2014 lxp [...] cpan.org - Subject changed from (no value) to '"inlinify" method triggers warnings when dealing with pseudoclasses'

Thu Jun 12 21:57:21 2014 lxp [...] cpan.org - Correspondence added

For what it's worth, I'm using the latest version of HTML::Query at this time (v0.08).

Fri Jun 13 21:24:52 2014 lxp [...] cpan.org - Correspondence added

Looking closely at the code within the "inlinify" method, it looks like CSS::Inliner already attempts to filter out the CSS that can't actually be inlined, but for some reason HTML::Query is still getting hit later: https://metacpan.org/source/KAMELKEV/CSS-Inliner-3945/lib/CSS/Inliner.pm#L296 #skip over the following psuedo selectors, these particular ones are not inlineable if ($selector =~ /(?:^|[\w\*]):(?:(active|focus|hover|link|visited|after|...))\b/io) { $self->_report_warning({ info => "The pseudo-class ':$1' cannot be supported inline" }); next; } This indicates to me that this is indeed a CSS::Inliner bug, and not an HTML::Query one. I'll update the ticket subject and see if I can work out later why this isn't working as expected.

Fri Jun 13 21:24:53 2014 lxp [...] cpan.org - Subject changed from '"inlinify" method triggers warnings when dealing with pseudoclasses' to 'some non-inline-able pseudo selectors not getting skipped'

Fri Jun 13 23:17:24 2014 lxp [...] cpan.org - Correspondence added

The selectors in my case which are causing HTML::Query to generate warnings (and therefore aren't being caught by the regular expression in CSS::Inliner) are as follows: input[class=button]:active input[class=button]:focus input[class=button]:hover input[type=reset]:active input[type=reset]:focus input[type=reset]:hover input[type=submit]:active input[type=submit]:focus input[type=submit]:hover #page-menu.pinned > li:first-of-type #page-menu.pinned > li:first-of-type > a .error .titlebox-content > p:last-of-type .rights-editor ul.rights-list li:nth-child(even) I suppose in the first nine cases, the square brackets are causing the regular expression not to apply. I don't know whether or not the ":first-of-type," ":last-of-type" and ":nth-child" pseudo classes are inline-able (and therefore whether they should be caught by the regex). This is probably the extent of contribution I can make to this issue without further comment from the distribution's author.

Mon Jun 16 20:33:25 2014 KAMELKEV [...] cpan.org - Correspondence added

That's interesting. I have never seen someone target psuedo-selectors like this, but it certainly makes sense that you could do this. Unlike other inlining applications out there (there are quite a few nowdays) this one is based on what we see in the real world rather than what is defined via spec. Due to this we still seemingly have an advantage over "the other guys". We are planning on doing a sprint soon to add support for shorthand selectors, when I add that support I will try modify this regex to behave as expected. The other psuedo-selectors you noted are possibly for us to support, we just haven't gotten there yet. I'm always looking for contributions... :) thanks, Kevin On Fri Jun 13 23:17:24 2014, LXP wrote: Show quoted text

> The selectors in my case which are causing HTML::Query to generate > warnings (and therefore aren't being caught by the regular expression in > CSS::Inliner) are as follows: > > input[class=button]:active > input[class=button]:focus > input[class=button]:hover > input[type=reset]:active > input[type=reset]:focus > input[type=reset]:hover > input[type=submit]:active > input[type=submit]:focus > input[type=submit]:hover > #page-menu.pinned > li:first-of-type > #page-menu.pinned > li:first-of-type > a > .error .titlebox-content > p:last-of-type > .rights-editor ul.rights-list li:nth-child(even) > > I suppose in the first nine cases, the square brackets are causing the > regular expression not to apply. I don't know whether or not the > ":first-of-type," ":last-of-type" and ":nth-child" pseudo classes are > inline-able (and therefore whether they should be caught by the regex). > > This is probably the extent of contribution I can make to this issue > without further comment from the distribution's author.

Mon Jun 16 20:33:25 2014 The RT System itself - Status changed from 'new' to 'open'

Tue Jun 17 21:39:23 2014 lxp [...] cpan.org - Correspondence added

On Tue Jun 17 10:33:25 2014, KAMELKEV wrote: Show quoted text

> We are planning on doing a sprint soon to add support for shorthand > selectors, when I add that support I will try modify this regex to > behave as expected.

Another possible fix could be as follows: It's evident through the warnings generated by HTML::Query that that distribution has logic to identify pseudo classes that it can't support. If HTML::Query were to expose that logic via a supports_pseudoclass($) method or similar, maybe CSS::Inliner could simply call that instead of defining its own logic. It sounds like the logic is currently (almost) duplicated across the two distributions, with the "almost" in this case causing the problem. (I realise that it could be more complex than this--I've only studied the code to the extent required to contribute to this discussion.) Show quoted text

> The other psuedo-selectors you noted are possibly for us to support, > we just haven't gotten there yet. I'm always looking for > contributions... :)

I notice that neither CSS::Inliner nor HTML::Query provide source code repository details in either the distribution metadata or the README. Is there such a publicly available repository, and what is your preferred method for receiving contributions?

Thu Aug 07 01:06:01 2014 KAMELKEV [...] cpan.org - Correspondence added

Both CPAN distros have source repos on github. I own one repo, and I have contributor access for the other. For this particular case I think it is going to make a lot more sense to simply fix the regex that is in place in Inliner. I don't see the error looking over it right now, but hopefully it's a minor oversight. You are right that Query and Inliner use different logic to catch these cases, but if you read over the two pieces you'll see they technically should be accomplishing the same thing. Inliner enumerates every single case that it doesn't support, whereas Query enumerates every case that it *does* support. In any case, there doesn't seem to be a hole with the enumerations, but rather the parsing/chopping that is occurring with the regex within Inliner. My first stop for solving this would be to start testing out that regex, but I still have to add shorthand support for the library sponsor first. Once that occurs, I will look at this. If you somehow have time you can submit a patch either here, or through github. Thanks for reporting this, I can see it would be annoying. -K On Tue Jun 17 21:39:23 2014, LXP wrote: Show quoted text

> On Tue Jun 17 10:33:25 2014, KAMELKEV wrote:

> > We are planning on doing a sprint soon to add support for shorthand > > selectors, when I add that support I will try modify this regex to > > behave as expected.

> > Another possible fix could be as follows: > > It's evident through the warnings generated by HTML::Query that that > distribution has logic to identify pseudo classes that it can't support. > > If HTML::Query were to expose that logic via a supports_pseudoclass($) > method or similar, maybe CSS::Inliner could simply call that instead of > defining its own logic. It sounds like the logic is currently (almost) > duplicated across the two distributions, with the "almost" in this case > causing the problem. > > (I realise that it could be more complex than this--I've only studied > the code to the extent required to contribute to this discussion.) >

> > The other psuedo-selectors you noted are possibly for us to support, > > we just haven't gotten there yet. I'm always looking for > > contributions... :)

> > I notice that neither CSS::Inliner nor HTML::Query provide source code > repository details in either the distribution metadata or the README. > Is there such a publicly available repository, and what is your > preferred method for receiving contributions?

Thu Aug 07 01:35:30 2014 KAMELKEV [...] cpan.org - Correspondence added

I am 99% sure the following patch would fix the issue you are having - but I need sample html files from you in order to create proper tests in order to exercise the code and make sure everything works the way it's supposed to. Please send over a file or two that causes multiple problems that you have discussed within this ticket, and we will test this and see if it works. If you want to try for yourself, I plan to test the following: diff --git a/lib/CSS/Inliner.pm b/lib/CSS/Inliner.pm index 6f4125f..6c15331 100755 --- a/lib/CSS/Inliner.pm +++ b/lib/CSS/Inliner.pm @@ -294,7 +294,7 @@ sub inlinify { my $declarations = $$entry{declarations}; #skip over the following psuedo selectors, these particular ones are not inlineable - if ($selector =~ /(?:^|[\w\*]):(?:(active|focus|hover|link|visited|after|before|selection|target|first-line|first-letter))\b/io) { + if ($selector =~ /(?:^|[\w\*\]]):(?:(active|focus|hover|link|visited|after|before|selection|target|first-line|first-letter))\b/io) { $self->_report_warning({ info => "The pseudo-class ':$1' cannot be supported inline" }); next; } On Thu Aug 07 01:06:01 2014, KAMELKEV wrote: Show quoted text

> Both CPAN distros have source repos on github. I own one repo, and I > have contributor access for the other. > > For this particular case I think it is going to make a lot more sense > to simply fix the regex that is in place in Inliner. I don't see the > error looking over it right now, but hopefully it's a minor oversight. > > You are right that Query and Inliner use different logic to catch > these cases, but if you read over the two pieces you'll see they > technically should be accomplishing the same thing. Inliner enumerates > every single case that it doesn't support, whereas Query enumerates > every case that it *does* support. In any case, there doesn't seem to > be a hole with the enumerations, but rather the parsing/chopping that > is occurring with the regex within Inliner. > > My first stop for solving this would be to start testing out that > regex, but I still have to add shorthand support for the library > sponsor first. Once that occurs, I will look at this. If you somehow > have time you can submit a patch either here, or through github. > > Thanks for reporting this, I can see it would be annoying. > > -K > > > > On Tue Jun 17 21:39:23 2014, LXP wrote:

> > On Tue Jun 17 10:33:25 2014, KAMELKEV wrote:

> > > We are planning on doing a sprint soon to add support for shorthand > > > selectors, when I add that support I will try modify this regex to > > > behave as expected.

> > > > Another possible fix could be as follows: > > > > It's evident through the warnings generated by HTML::Query that that > > distribution has logic to identify pseudo classes that it can't > > support. > > > > If HTML::Query were to expose that logic via a > > supports_pseudoclass($) > > method or similar, maybe CSS::Inliner could simply call that instead > > of > > defining its own logic. It sounds like the logic is currently > > (almost) > > duplicated across the two distributions, with the "almost" in this > > case > > causing the problem. > > > > (I realise that it could be more complex than this--I've only studied > > the code to the extent required to contribute to this discussion.) > >

> > > The other psuedo-selectors you noted are possibly for us to > > > support, > > > we just haven't gotten there yet. I'm always looking for > > > contributions... :)

> > > > I notice that neither CSS::Inliner nor HTML::Query provide source > > code > > repository details in either the distribution metadata or the README. > > Is there such a publicly available repository, and what is your > > preferred method for receiving contributions?

Thu Aug 07 03:50:37 2014 lxp [...] cpan.org - Correspondence added

On Thu Aug 07 15:35:30 2014, KAMELKEV wrote: Show quoted text

> Please send over a file or two that causes multiple problems that you > have discussed within this ticket, and we will test this and see if it > works.

The attached HTML file triggers the following warnings for me: Pseudoclass :hover not supported at .../HTML/Query.pm line 284. Pseudoclass :hover not supported at .../HTML/Query.pm line 284. Pseudoclass :hover not supported at .../HTML/Query.pm line 284. Pseudoclass :focus not supported at .../HTML/Query.pm line 284. Pseudoclass :focus not supported at .../HTML/Query.pm line 284. Pseudoclass :focus not supported at .../HTML/Query.pm line 284. Pseudoclass :last-of-type not supported at .../HTML/Query.pm line 284. Pseudoclass :nth-child not supported at .../HTML/Query.pm line 284. Pseudoclass :first-of-type not supported at .../HTML/Query.pm line 284. Pseudoclass :first-of-type not supported at .../HTML/Query.pm line 284. Pseudoclass :focus not supported at .../HTML/Query.pm line 284. Pseudoclass :focus not supported at .../HTML/Query.pm line 284. Pseudoclass :focus not supported at .../HTML/Query.pm line 284. Pseudoclass :hover not supported at .../HTML/Query.pm line 284. Pseudoclass :hover not supported at .../HTML/Query.pm line 284. Pseudoclass :hover not supported at .../HTML/Query.pm line 284. Pseudoclass :active not supported at .../HTML/Query.pm line 284. Pseudoclass :active not supported at .../HTML/Query.pm line 284. Pseudoclass :active not supported at .../HTML/Query.pm line 284. On Thu Aug 07 15:06:01 2014, KAMELKEV wrote: Show quoted text

> Both CPAN distros have source repos on github. I own one repo, and I > have contributor access for the other. > ... > If you somehow have time you can submit a patch either here, or > through github.

I believe that you're referring to these repositories: https://github.com/kamelkev/CSS-Inliner https://github.com/abw/HTML-Query If that's the case, I'll see if I can get a chance to abstract the detection logic into a single place (thus preventing the need to keep two pieces of code synchronised in future).

Subject:

pseudoclass-not-supported.html

Message body is not shown because it is too large.

Thu Aug 07 18:49:34 2014 KAMELKEV [...] cpan.org - Correspondence added

Ok I received your html, I will generate a test file from this that exercises the cases outlined here. I got distracted in my last reply, the repos are: https://github.com/kamelkev/HTML-Query https://github.com/kamelkev/CSS-Inliner I take contributions to my forked repo of HTML-Query and then push those back over to Andy's when it's release time. I'm skeptical about the plan of creating a test method within HTML::Query that will jive with the design philosophy, but if you can get it to line up I'm more than willing to look. Of issue is the different philosophies present in each of the libraries, but I'm not entirely sure what you have in mind so maybe you can make it work. On Thu Aug 07 03:50:37 2014, LXP wrote: Show quoted text

> On Thu Aug 07 15:35:30 2014, KAMELKEV wrote:

> > Please send over a file or two that causes multiple problems that you > > have discussed within this ticket, and we will test this and see if it > > works.

> > The attached HTML file triggers the following warnings for me: > > Pseudoclass :hover not supported at .../HTML/Query.pm line 284. > Pseudoclass :hover not supported at .../HTML/Query.pm line 284. > Pseudoclass :hover not supported at .../HTML/Query.pm line 284. > Pseudoclass :focus not supported at .../HTML/Query.pm line 284. > Pseudoclass :focus not supported at .../HTML/Query.pm line 284. > Pseudoclass :focus not supported at .../HTML/Query.pm line 284. > Pseudoclass :last-of-type not supported at .../HTML/Query.pm line 284. > Pseudoclass :nth-child not supported at .../HTML/Query.pm line 284. > Pseudoclass :first-of-type not supported at .../HTML/Query.pm line 284. > Pseudoclass :first-of-type not supported at .../HTML/Query.pm line 284. > Pseudoclass :focus not supported at .../HTML/Query.pm line 284. > Pseudoclass :focus not supported at .../HTML/Query.pm line 284. > Pseudoclass :focus not supported at .../HTML/Query.pm line 284. > Pseudoclass :hover not supported at .../HTML/Query.pm line 284. > Pseudoclass :hover not supported at .../HTML/Query.pm line 284. > Pseudoclass :hover not supported at .../HTML/Query.pm line 284. > Pseudoclass :active not supported at .../HTML/Query.pm line 284. > Pseudoclass :active not supported at .../HTML/Query.pm line 284. > Pseudoclass :active not supported at .../HTML/Query.pm line 284. > > > On Thu Aug 07 15:06:01 2014, KAMELKEV wrote:

> > Both CPAN distros have source repos on github. I own one repo, and I > > have contributor access for the other. > > ... > > If you somehow have time you can submit a patch either here, or > > through github.

> > I believe that you're referring to these repositories: > > https://github.com/kamelkev/CSS-Inliner > https://github.com/abw/HTML-Query > > If that's the case, I'll see if I can get a chance to abstract the > detection logic into a single place (thus preventing the need to keep > two pieces of code synchronised in future).

Tue Sep 02 23:26:32 2014 KAMELKEV [...] cpan.org - Correspondence added

New version uploaded to CPAN. It will take a day or two to clear. In the meantime you can pull down the latest directly off of my github account: https://github.com/kamelkev/CSS-Inliner https://github.com/kamelkev/HTML-Query Thanks for reporting this, that was annoying. -K On Thu Aug 07 03:50:37 2014, LXP wrote: Show quoted text

> On Thu Aug 07 15:35:30 2014, KAMELKEV wrote:

> > Please send over a file or two that causes multiple problems that you > > have discussed within this ticket, and we will test this and see if it > > works.

> > The attached HTML file triggers the following warnings for me: > > Pseudoclass :hover not supported at .../HTML/Query.pm line 284. > Pseudoclass :hover not supported at .../HTML/Query.pm line 284. > Pseudoclass :hover not supported at .../HTML/Query.pm line 284. > Pseudoclass :focus not supported at .../HTML/Query.pm line 284. > Pseudoclass :focus not supported at .../HTML/Query.pm line 284. > Pseudoclass :focus not supported at .../HTML/Query.pm line 284. > Pseudoclass :last-of-type not supported at .../HTML/Query.pm line 284. > Pseudoclass :nth-child not supported at .../HTML/Query.pm line 284. > Pseudoclass :first-of-type not supported at .../HTML/Query.pm line 284. > Pseudoclass :first-of-type not supported at .../HTML/Query.pm line 284. > Pseudoclass :focus not supported at .../HTML/Query.pm line 284. > Pseudoclass :focus not supported at .../HTML/Query.pm line 284. > Pseudoclass :focus not supported at .../HTML/Query.pm line 284. > Pseudoclass :hover not supported at .../HTML/Query.pm line 284. > Pseudoclass :hover not supported at .../HTML/Query.pm line 284. > Pseudoclass :hover not supported at .../HTML/Query.pm line 284. > Pseudoclass :active not supported at .../HTML/Query.pm line 284. > Pseudoclass :active not supported at .../HTML/Query.pm line 284. > Pseudoclass :active not supported at .../HTML/Query.pm line 284. > > > On Thu Aug 07 15:06:01 2014, KAMELKEV wrote:

> > Both CPAN distros have source repos on github. I own one repo, and I > > have contributor access for the other. > > ... > > If you somehow have time you can submit a patch either here, or > > through github.

> > I believe that you're referring to these repositories: > > https://github.com/kamelkev/CSS-Inliner > https://github.com/abw/HTML-Query > > If that's the case, I'll see if I can get a chance to abstract the > detection logic into a single place (thus preventing the need to keep > two pieces of code synchronised in future).

Tue Sep 02 23:28:32 2014 KAMELKEV [...] cpan.org - Correspondence added

Fixed in CSS::Inliner 3948 and HTML::Query 0.09

Tue Sep 02 23:28:33 2014 KAMELKEV [...] cpan.org - Status changed from 'open' to 'resolved'