-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Text Replacer example #612
base: main
Are you sure you want to change the base?
Conversation
return output; | ||
} | ||
|
||
// Use var to avoid "Identifier 'REGEXP_SPECIAL_CHARACTERS' has already been |
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.
Oh wow. I didn't realise this runs non-ESM code and runs in the top-scope.
Everything in this file is being redeclared always. Would it be worth putting it in an IIFE? Is that too complex?
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.
+1
var REGEXP_SPECIAL_CHARACTERS = /[.(){}^$*+?[\]\\]/g; | ||
/** Sanitize user input to prevent unexpected behavior during RegExp execution */ | ||
function escapeRegExp(pattern) { | ||
return pattern.replace(REGEXP_SPECIAL_CHARACTERS, "\\$&") |
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.
I highly suspect this might not be all regex chars. Maybe call out that this is for example purposes only?
</div> | ||
</form> | ||
|
||
<script src="popup.js"></script> |
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.
Can you use type="module"
? Then you can use top-level await in the code, rather than the awkward loading and call to loadFormData
.
}); | ||
}); | ||
|
||
document.getElementById('clear').addEventListener('click', (event) => { |
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.
You can get rid of this and change the <button>
to be an <input type="reset" />
.
const inputs = form.querySelectorAll('input[type=text]'); | ||
|
||
const patterns = [...inputs].reduce((acc, input, index) => { | ||
const outerIndex = index >> 1; |
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.
Do a real division and round it. Sorry, but we should be clear to novices.
// https://developers.google.com/open-source/licenses/bsd | ||
|
||
// Replace text on the page using a static list of patterns | ||
function textReplacer(replacements) { |
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.
I think having this method is a bit confusing. I'd just move its two lines into the callback for the fetcher, and expand the comment:
// Get the patterns to replace from storage, then build a regex from them and replace all text on the page.
chrome.runtime.onInstalled.addListener(() => { | ||
registerContextMenus(); | ||
}); |
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.
During extension update, will this cause an error by trying to re-register context menu items?
} | ||
|
||
chrome.contextMenus.onClicked.addListener((info, tab) => { | ||
if (info.menuItemId == 'replace-text-menuitem') { |
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.
extract "replace-text-menuitem" into a constant
chrome.contextMenus.onClicked.addListener((info, tab) => { | ||
if (info.menuItemId == 'replace-text-menuitem') { | ||
replaceText(tab.id); | ||
} |
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.
I'd normally recommend throwing an assert in here, since this is the only id we ever expect. Similarly for commands.
|
||
function buildReplacementRegex(source) { | ||
const output = []; | ||
for (var i = 0; i < source.length; i++) { |
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.
for (var i = 0; i < source.length; i++) { | |
for (let i = 0; i < source.length; i++) { |
return output; | ||
} | ||
|
||
// Use var to avoid "Identifier 'REGEXP_SPECIAL_CHARACTERS' has already been |
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.
+1
function replaceText(replacements) { | ||
let node; | ||
const nodeIterator = document.createNodeIterator(document.body, NodeFilter.SHOW_TEXT); | ||
while (node = nodeIterator.nextNode()) { |
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.
can we declare node here? We only need it in the body
chrome.storage.sync.get(['patterns'], function(data) { | ||
textReplacer(data.patterns); | ||
}); |
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.
We're going to be gradually encouraging developers to avoid storage access by content scripts unless crucial. WDYT about changing this to a message to the background page? (I don't feel strongly)
}, | ||
"commands": { | ||
"replace-text": { | ||
"description": "Replace text on the current page." |
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.
Doesn't this need a default keybinding?
"64": "icons/icon64.png" | ||
}, | ||
"action": { | ||
"default_title": "Show an alert", |
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 this an alert?
README.md |
+1 |
@sebastianbenz @oliverdunk what should we do with these older PRs? |
Adds a sample extension called "Text Replacer." As the name implies, this extension will replace a string in the page with a custom string. Currently supports 5 replacements.