-
Notifications
You must be signed in to change notification settings - Fork 120
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
Whole word scanning #1330
Whole word scanning #1330
Conversation
View Playwright Report (note: open the "playwright-report" artifact) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any upper bound for how long a word is/how far backwards it can seek? I'm not seeing anything and this could lead to quite a lot of trouble if a user tries to scan an enormous string. Even a huge limit like 1000 characters would probably be a good idea.
@Kuuuube it uses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, will approve when tests are fixed. test/options-util.test.js
needs the version bumped from 50 to 51.
Words in quotes might not be scannable. Also not sure why the first word in the example here can't be scanned: Probably seeking back into other HTML elements and concatenating. Layout-aware scanning doesn't seem to impact it. |
Might want to have it pull the sentence termination characters and break on any of them. Though there is also the issue of should it stop on |
Might be the same or similar issue to what i demonstrated on discord. |
I think it's safe to just look one character before the quote and break if it's a whitespace, and not otherwise. Or rather, look for a whitespace and any combination of quotes, to accommodate the case if the string is doubly quoted. |
It now works on double quoted words but not on single quoted words. |
Otherwise lgtm |
Okay done I followed your advice and peeked at the character before the single quotes to see if it's an apostrophe or wrapped in single quotes |
Fixes #889