Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX] First-child and last-child selectors are broken #293

Merged
merged 1 commit into from May 10, 2016

Conversation

oliverklee
Copy link
Contributor

Fixes #286

@oliverklee
Copy link
Contributor Author

I've used the XPath equivalents as listed on https://en.wikibooks.org/wiki/XPath/CSS_Equivalents.

@bramley: Could you please test this PR and also verify it by reading?

@bramley
Copy link
Contributor

bramley commented Dec 28, 2015

The XPaths are not correct. The first-child one is selecting all elements that are the first child of their parent. Similarly the last-child is selecting all elements that are the last child of their parent.

The Symfony css selector package generates this XPath for p:first-child

descendant-or-self::*/*[name() = 'p' and (position() = 1)]

While that could be generated by the preg_replace, the XPath expression can be more complex so I don't think it is easy (or even possible) to process a general expression with a preg_replace, e.g. p.red:first-child generates

descendant-or-self::div/*[@class and contains(concat(' ', normalize-space(@class), ' '), ' red ') and (name() = 'p') and (position() = 1)]

As there can be subtle differences between similar XPath expressions, I'm not sure it is a good idea to try to implement css to XPath conversion without having a good knowledge of both. Particularly as there is a proven package that already does that. Wouldn't it be better to treat this as "missing functionality" or as a bug and fix it by using the Symfony component?

@oliverklee oliverklee added the bug label May 7, 2016
@oliverklee oliverklee added this to the 1.1.0 milestone May 7, 2016
@oliverklee
Copy link
Contributor Author

Hi @bramley, thanks for the remarks. I've fixed the code and added some tests that fail with the old version of the patch. Could you please re-review? Thanks!

@bramley
Copy link
Contributor

bramley commented May 7, 2016

Yes, this looks better.
You might want to change these two test descriptions to refer to the div element, not p

        'P:first-child matches not first child with mismatching tag'
            => ['div:first-child {' . $styleRule . '} ', '#<p class="p-1">#'],

        'P:last-child not matches last child with mismatching tag'
            => ['div:last-child {' . $styleRule . '} ', '#<p class="p-3">#'],

and maybe change "matches not" to "does not match"

@oliverklee
Copy link
Contributor Author

@bramley Good catch! Like this (re-pushed)?

@oliverklee oliverklee merged commit 82a67a1 into master May 10, 2016
@oliverklee oliverklee deleted the bugfix/first-last-child branch May 10, 2016 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants