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

Escape `\n` on `findElementLocation` #1792

Closed
molant opened this Issue Jan 30, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@molant
Copy link
Member

molant commented Jan 30, 2019

I've run the extension in this page with the defaults and I'm getting a bunch of errors :(

Uncaught (in promise) DOMException: Failed to execute 'querySelectorAll' on 'Document': 'meta[property="og:description"][content="Pull request checklist
Make sure you:

 Signed the Contributor License Agreement
 Followed the commit message guidelines

For non-trivial changes, please make sure you also:

 Added/Updated related..."]' is not a valid selector.
    at AsyncHTMLDocument.querySelectorAll (file:///C:/Users/molant/Downloads/extension-browser-pr-1596/content-script/webhint.js:61467:51)
    at Object.exports.findElementLocation (file:///C:/Users/molant/Downloads/extension-browser-pr-1596/content-script/webhint.js:33387:50)
    at async Object.exports.findProblemLocation (file:///C:/Users/molant/Downloads/extension-browser-pr-1596/content-script/webhint.js:33446:29)
    at async CompatHTML.<anonymous> (file:///C:/Users/molant/Downloads/extension-browser-pr-1596/content-script/webhint.js:63373:30)
    at async file:///C:/Users/molant/Downloads/extension-browser-pr-1596/content-script/webhint.js:4832:40

This is probably not related to the extension itself.

Originally posted by @molant in #1596 (comment)

@molant molant added the type:bug label Jan 30, 2019

@antross antross self-assigned this Jan 30, 2019

@antross antross added this to the 1902-1 milestone Jan 30, 2019

@antross

This comment has been minimized.

Copy link
Member

antross commented Jan 30, 2019

When trying to repro the issue I hit another case as well. Apparently the HTML parser allows % as an attribute name, but that also fails when building a selector (and when trying to use setAttribute for that matter).

hint.ts:95 Uncaught (in promise) DOMException: Failed to execute 'querySelectorAll' on 'Document': 'meta[class="js-ga-set"][name="userId"][content="e7b44cb91a0f47c26e245ec48e6a5652"][%=""]' is not a valid selector.
    at AsyncHTMLDocument.querySelectorAll (file:///C:/Users/tross/Documents/GitHub/hint/packages/extension-browser/dist/bundle/content-script/webhint.js:61467:51)
    at Object.exports.findElementLocation (file:///C:/Users/tross/Documents/GitHub/hint/packages/extension-browser/dist/bundle/content-script/webhint.js:33387:50)
    at async Object.exports.findProblemLocation (file:///C:/Users/tross/Documents/GitHub/hint/packages/extension-browser/dist/bundle/content-script/webhint.js:33446:29)
    at async CompatHTML.<anonymous> (file:///C:/Users/tross/Documents/GitHub/hint/packages/extension-browser/dist/bundle/content-script/webhint.js:63373:30)
    at async file:///C:/Users/tross/Documents/GitHub/hint/packages/extension-browser/dist/bundle/content-script/webhint.js:4832:40
@antross

This comment has been minimized.

Copy link
Member

antross commented Jan 30, 2019

Rather than chase down each possible invalid selector, I'm going to fix the \n case and add a try/catch to ensure we handle potential invalid selectors gracefully. FWIW this whole code path should become obsolete after we do #1707 as we'll get location information from each element when we use parser-html to load the DOM snapshot.

@molant

This comment has been minimized.

Copy link
Member Author

molant commented Jan 30, 2019

Sounds good to me. More reason to do #1707 sooner rather than later.

antross added a commit to antross/hint that referenced this issue Jan 30, 2019

Fix: Handle newlines in attribute values
Fix webhintio#1792

Also gracefully handle invalid selectors when generating locations.

@antross antross referenced this issue Jan 30, 2019

Merged

Fix: Handle newlines in attribute values #1802

3 of 3 tasks complete

antross added a commit that referenced this issue Jan 31, 2019

Fix: Handle newlines in attribute values
Fix #1792

Also gracefully handle invalid selectors when generating locations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment