support :nth-child(2n + 1) syntax [whitespace around operator]

[jkillian]

Fixes #20

Not sure if this is the right way to fix and test this fix, but wanted to try and make a PR to help out! I wasn't sure if I was supposed to use the WSP variable or just \s to indicate whitespace, so I tried to use the same methodology as the surrounding code.

I also added a few test cases - I was a little confused about what file to add tests in, so let me know if this is the wrong place.

Also, feel free to discard my PR all together if it's easier to just do things yourself!

[dperini]

Notice that the Config.SIMPLE_NOT flag has been marked obsolete internally and it will disappear as soon as possible to simplify the code and remove non-standard functionality. Currently It cannot be removed since there are still dependencies with the syntax validator.
It is probably due for obsolescence in version 2.1.0 of nwsapi.

[jkillian]

Hmm, I think there are some subtleties going on here that are a bit tricky. For example, this line handles one space around a + sign but not around a - sign:

nwsapi/src/nwsapi.js

Line 769 in 2a77b80

selector = selector.replace(/\s?([>+~])\s?/g, '$1');

So I don't think the normalization is working as intended? For example, take a look at the state of selector when we get to the point where we are matching against strcut_2 (which is part of strcut_p) :

[jkillian]

I believe I initially misdiagnosed the situation, although the regex validation with SIMPLE_NOT is part of the issue, selectors such as p:nth-child(2n - 1) or p:nth-child(2n + 1) don't work in either mode.

So it seems to me that fixing the normalization might fix the root cause of this issue (with perhaps a few other small tweaks needed?)

[jkillian]

I've opened a PR with WPT to see if they'll add these selectors to their tests: web-platform-tests/wpt#12561

[jkillian]

I added some more test cases which demonstrate why * is necessary right now. I did make one improvement by updating the selector = selector.replace(/\s?([>+~])\s?/g, '$1'); line; I believe it should be safe to use \s* there, though I don't have a great understanding of regex backtracking vulnerabilities.

[dperini]

@jkillian the line you mentioned above:

selector = selector.replace(/\s?([>+~])\s?/g, '$1'); 

is not part of a check on (an+b) parameters of structural pseudo-classes it is only a check targeting combinators, handling only one optional space before or after selectors and considering that the space character (0x20) is itself a combinator (one of the four). I don't think the problem is there.

However the point you made is correct and the relevant fact I need to check is the reason white-space characters are not always coalesced to a single space.

Thank you again for reviewing that part of the code.

[dperini]

@jkillian
FYI part of the normalization I talked about is performed by these lines:

1390 // normalize selector
1391 selector = selector.
1392 replace(/\x00|\$/g, '\ufffd').
1393 replace(REX.TrimSpaces, '');

but looking to it seems the "coalescing" part is missing.

[jkillian]

I added a naive solution where we combine all whitespace into a single space.

However I think this is likely the wrong implementation. Since a selector like this: div[attr="five . spaces"] is not the same as div[attr="one space"]. And there may be other cases too where the behavior is wrong.

That said, all the test cases in W3C-Selector-tests-nwsapi.html are passing, though I suspect some tests in WPT would fail.

[dperini]

@jkillian I will first say that the issue you brought up does spread beyond the pseudo-class parameters and needs some change also in the regular expressions currently used to cover other cases. Normalization of the input selector string is the key here.

There are a couple of problems that must be solved to shape a final solution for this issue.

The first is the one you mentioned, the input selector string normalization requires a clever RegExp.
The second problem I see is that the patches need to consider passing other tests, namely:

• the /test/css3-escapes in nwsapi
• the /dom/nodes/ParentNode-querySelector-escapes.html in web-platform-test

Both files execute the same series of tests to check nwsapi ability to handle edge case escape sequences in selector strings. There is one duplicate test in the WPT version but it currently pass all of them while in the nwsapi version you must manually review results in the browser console.

To run the WPT test in nwsapiyou must first run /test/w3c-test.sh to launch the new PHP7 embedded web server then connect to localhost:8000.

Since I am publishing the next version of nwsapi soon (version 2.1.0) I prefer to back port the needed RegExp I have been working on. It can be surely improved but for now it is clever enough to avoid trashing values in attributes and the like as you already realized, this is the current one I have:

RegExp('(\\s+)(?=(?:[^"]*["][^"]*["])*[^"]*$)' + "(?=(?:[^']*['][^']*['])*[^']*$)", 'g'),

This RE will only change multiple white space chars not enclosed in single or double quotes, maybe I will need to add back-ticks "`" too but I need to review the specification and see what is required and what is not.

At the moment the above RE works great for the task, so I am preparing the needed commits.

I will surely give you the deserved credits for bringing my attention on this bug fix as soon as I commit the changes.

Thank you again for your help with this.

[jkillian]

Hi @dperini, thank you for the reply again! Yes, I think it's best if we use your careful work to handle the whitespace issue. Once you have that in, we can add some test cases like the one in this PR to see if any further work is needed.

If 2.1.0 is going to be released soon, we could wait until then to fix this bug - it might make things simpler overall. Your choice!

(As a side comment, would you consider adding a section to your readme about the necessary tests that must be run to check changes to the codebase? Your information above is helpful and I think it might help people in the future to have the info in the readme.)

[jkillian]

Fixed more correctly by 9dfcc2a, so closing this out!