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

fix quote regexp #16

Closed
wants to merge 1 commit into from
Closed

fix quote regexp #16

wants to merge 1 commit into from

Conversation

lifaon74
Copy link

@lifaon74 lifaon74 commented Dec 9, 2020

Hi, first thanks for your excellent library !

I found a strange behaviour when using attribute selectors:

a[attr="abcde"][attr="123"]

link

The second attribute returns:

	{
		"type": "attribute",
		"content": "[attr=\"§\"]",
		"name": "attr",
		"operator": "=",
		"value": "\"§\"",
		"pos": [
			11,
			21
		]
	}

Note the §.

I found and fixed the issue on the quote replace regexp.

@LeaVerou
Copy link
Owner

Hi there,
Thank you for looking into this!
Could you elaborate on this a bit? What was causing the issue and how did you fix it?
It would help me review the PR, as the answers are not immediately obvious to me by looking at your edit.

@lifaon74
Copy link
Author

Ok, so as you commented:

// FIXME Does not account for escaped backslashes before a quote
selector = selector.replace(/(['"])(\\\1|.)+?\1/g, (str, quote, content, start) => {

Actually this regexp is incorrect in 2 points:

  • (\\\1|.)+ this puts only the last matching char in content. ((?:\\\1|.)+) is correct
    • this is what triggers the reported bug: link
  • you didn't handle escaped quotes, as it is actually not a simple task (you commented about it)

A more accurate regexp is: /(?:"((?:[^"\\]|\\.)*)")|(?:'((?:[^'\\]|\\.)*)')/g

  • split: "((?:[^"\\]|\\.)*)" -> select in quotes and handle backslashes

@lifaon74
Copy link
Author

lifaon74 commented Dec 10, 2020

Note: the regexp can't handle \\\\ (ex: "ab\\\\"hg"). Actually I guess a regexp can't handle properly every escapes and quotes in string. For this a proper parser may be used.

@lifaon74
Copy link
Author

lifaon74 commented Jan 5, 2021

Hi, any news ?

@jrandolf
Copy link
Collaborator

Fixed in #57

@jrandolf jrandolf closed this Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants