Fix CSS selector support for numeric IDs and escape sequences #150
Conversation
|
|
||
| private (string strategy, string value) ParseCssSelector(string cssSelector) | ||
| { | ||
| if (cssSelector.StartsWith("#")) |
There was a problem hiding this comment.
This is not sufficient to judge that it is a simple CSS ID selector. To be sure, shouldn't a regex be used that checks that the complete cssSelector string matches a simple ID selector? It may be that the user sends a compound selector instead, and we should have a proper error message in that case that this is not supported.
The same comment holds for all parses below. To check that a regex matches the string fully, start with ^ and end with $.
|
Would it be possible to add a unit test to e.g. |
…o check the current issue
|
Actually I found out that my approach to fixing the CSS selector inside the controller was a mistake. I've refactored this to handle CSS selector parsing in ConditionParser instead. |
| } | ||
|
|
||
| var nameMatch = Regex.Match(cssSelector, @"\*?\[name\s*=\s*""(.+?)""\]", RegexOptions.IgnoreCase); | ||
| var nameMatch = Regex.Match(cssSelector, @"^\*?\[name\s*=\s*""(.+?)""\]$", RegexOptions.IgnoreCase); |
There was a problem hiding this comment.
Couldn't this still match [name="test"][class="test2"] because of the .+ expression? If so, I suggest [^""]+ instead. Please add a unit test to validate that [name="test"][class="test2"] throws as not supported.
| /// - Multiple selectors are not supported. | ||
| /// </summary> | ||
| private static Regex SimpleCssIdSelectorRegex = new Regex(@"^#(?<name>(?<nmchar>[_a-z0-9-]|[\240-\377]|(?<escape>\\[^\r\n\f0-9a-f]))+)$", RegexOptions.Compiled | RegexOptions.IgnoreCase); | ||
| private static Regex SimpleCssIdSelectorRegex = new Regex(@"^#(?<name>(?<nmchar>[_a-z0-9-]|[\240-\377]|(?<escape>\\[^\r\n\f0-9a-f])|(?<unicode>\\[0-9a-fA-F]{1,6}\s?))+)$", RegexOptions.Compiled | RegexOptions.IgnoreCase); |
There was a problem hiding this comment.
Shouldn't the \s? part be omitted here?
| /// - Multiple selectors are not supported. | ||
| /// </summary> | ||
| private static Regex SimpleCssClassSelectorRegex = new Regex(@"^\.(?<ident>-?(?<nmstart>[_a-z]|[\240-\377])(?<nmchar>[_a-z0-9-]|[\240-\377]|(?<escape>\\[^\r\n\f0-9a-f]))*)$", RegexOptions.Compiled | RegexOptions.IgnoreCase); | ||
| private static Regex SimpleCssClassSelectorRegex = new Regex(@"^\.(?<ident>-?(?<nmstart>[_a-z]|[\240-\377])(?<nmchar>[_a-z0-9-]|[\240-\377]|(?<escape>\\[^\r\n\f0-9a-f])|(?<unicode>\\[0-9a-fA-F]{1,6}\s?))*)$", RegexOptions.Compiled | RegexOptions.IgnoreCase); |
There was a problem hiding this comment.
Shouldn't the \s? part be omitted here?
| /// - ~= or |= not supported. | ||
| /// </summary> | ||
| private static Regex SimpleCssAttributeSelectorRegex = new Regex(@"^\*?\[\s*(?<ident>-?(?<nmstart>[_a-z]|[\240-\377])(?<nmchar>[_a-z0-9-]|[\240-\377])*)\s*=\s*(?<string>(?<string1>""(?<string1value>([^\n\r\f\\""]|(?<escape>\\[^\r\n\f0-9a-f]))*)"")|(?<string2>'(?<string2value>([^\n\r\f\\']|(?<escape>\\[^\r\n\f0-9a-f]))*)'))\s*\]$", RegexOptions.Compiled | RegexOptions.IgnoreCase); | ||
| private static Regex SimpleCssAttributeSelectorRegex = new Regex(@"^\*?\[\s*(?<ident>-?(?<nmstart>[_a-z]|[\240-\377])(?<nmchar>[_a-z0-9-]|[\240-\377])*)\s*=\s*(?<string>(?<string1>""(?<string1value>([^\n\r\f\\""]|(?<escape>\\[^\r\n\f0-9a-f])|(?<unicode>\\[0-9a-fA-F]{1,6}\s?))*)"")|(?<string2>'(?<string2value>([^\n\r\f\\']|(?<escape>\\[^\r\n\f0-9a-f])|(?<unicode>\\[0-9a-fA-F]{1,6}\s?))*)'))\s*\]$", RegexOptions.Compiled | RegexOptions.IgnoreCase); |
There was a problem hiding this comment.
Shouldn't the \s? part be omitted here?
| [Test] | ||
| public void ParseCondition_CssSelectorWithSimpleNumericId_ReturnsAutomationIdCondition() | ||
| { | ||
| var cssSelector = @"#\31 "; |
There was a problem hiding this comment.
Isn't "#\31" without the trailing space a better example for this case?
| { | ||
| public class ConditionParserTests | ||
| { | ||
| [TestCase("[name=\"2\"]")] |
There was a problem hiding this comment.
Why were the ParseCondition_ByCssAttributeName_ReturnsCondition test cases removed?
|
Please squash the commits in this merge request together. E.g. with |
2 participants
Fixes #138