Fix name selector to allow other characters#11537
Fix name selector to allow other characters#11537christian-bromann merged 1 commit intowebdriverio:mainfrom
Conversation
erwinheitzman
left a comment
There was a problem hiding this comment.
Thank you for the contribution, looks good to me with the exception of a small request for change as the testcase is confusing.
94e7698 to
de518b1
Compare
erwinheitzman
left a comment
There was a problem hiding this comment.
I just have a question, I assume it is correct because the tests are passing but from looking at the code it feels off somehow and I just want to make sure we are doing the right thing 🙂
| } | ||
| using = 'name' | ||
| value = match[2] | ||
| value = match[3] || match[5] |
There was a problem hiding this comment.
Just to be sure, I can't really tell how the match could have changed. Perhaps I am overlooking something but could you please explain this so we can be sure this is indeed correct?
The reason I ask is because it is still a single match right?
There was a problem hiding this comment.
There are two possibilities: matching with double quotes or matching with single quotes. Both have different capture groups (group 3 and group 5) in the regular expression. It would be possible to make it a single capture with a single regular expression as before, but then the regular expression would also match e.g. [name="test'] which is an invalid expression. The other option is to make 2 different regular expressions, one for double quotes and one for single quotes. I thought this would be easiest: get the capture from the double quotes if it exists (capture 3), otherwise get the capture from the single quotes (capture 5).
There was a problem hiding this comment.
I see now, I feel like we should optimize the regex a bit.
\[name=(?:"(.[^"]*)"|'(.[^']*)')]It would be more performant and returns a match on the full regex and 1 group.
What do you think about this?
EDIT: the reason this is more performant is because it uses a non-capturing group to match A or B and only create one group of the result
There was a problem hiding this comment.
Thanks, pushed an update
christian-bromann
left a comment
There was a problem hiding this comment.
LGTM 👍
Wdyt @erwinheitzman ?
I would like to optimize the regex first if you don't mind 🙂 EDIT: see #11537 (comment) |
|
@aristotelos can we address the comment mentioned by @erwinheitzman ? |
Fix the `name` selector strategy to allow other characters, such as
parentheses.
For example, using `$('[name="test (Test)"]')` will now
correctly result in using the `name` selector strategy instead of using
the `css selector` strategy.
Fixes issue webdriverio#11553.
de518b1 to
68c0456
Compare
|
@erwinheitzman wdyt? |
@christian-bromann i think we can merge it and I can optimize it after |
|
Congratulations @aristotelos on your remarkable first contribution to WebdriverIO! This project thrives on the invaluable involvement of our community, and we are truly grateful for your contribution. We eagerly anticipate witnessing more of your exceptional work, so please don't hesitate to inform us if we can assist you in identifying intriguing areas where you can make further contributions. Join our lively Discord channel and reach out to us; we would be delighted to connect with you. Your efforts are deeply appreciated, and we extend our heartfelt gratitude to you. 🙏 ❤️ |
Fix the
nameselector strategy to allow other characters, such as parentheses.For example, using
$('[name="test (Test)"]')will now correctly result in using thenameselector strategy instead of using thecss selectorstrategy.Fixes #11553
Proposed changes
Fix the
nameselector strategy to allow other characters, such as parentheses.For example, using
$('[name="test (Test)"]')will now correctly result in using thenameselector strategy instead of using thecss selectorstrategy.Fixes #11553
Types of changes
Checklist
Further comments
Reviewers: @webdriverio/project-committers