Use MutationObserver to listen for DOM changes. #736
Conversation
- Only run pattern detection on changed nodes. - Start the content script at document_start, otherwise we miss the window.load event
maxxcrawford
requested review from
groovecoder and
maxxcrawford
and removed request for
groovecoder
|
Hi, so, I knew this would take some time to review but I didn't quite expect this to hang around for more than 7 months. Are there any plans to merge this PR? |
…to mozilla-main
|
Hi @maxxcrawford, I merged the latest from the upstream main branch into my own branch (and PR) so that there aren't any merge conflicts.
Cheers, Matthijs |
|
@codemist Can you incorporate this PR into your iframe work? I'd love to merge this in. Let's sync and see if we should merge this in as is first, and you solve conflicts from your side. |
@mrwensveen Thanks for the work on this. We will incorporate this in our next release! |
|
Thanks @codemist! Should be fixed now. |
There was a problem hiding this comment.
@mrwensveen Thank you for this PR! This looks good to ship. I had one nit comment, but we can resolve it in a future PR.
Commented using conventional comments language
| const OBSERVER_ATTRIBUTES = [ | ||
| "action", "aria-label", "class", | ||
| "data-action", "data-bfa-network", "data-destination", "data-login-with-facebook", | ||
| "data-oauthserver", "data-partner", "data-tag", "data-test-id", "data-tracking", | ||
| "href", "id", "title"]; |
There was a problem hiding this comment.
praise: This is so much cleaner to watch these attrs, rather the whole, dang DOM.
| } else { | ||
| contentScriptInit(true); | ||
| // contentScriptInit(true); | ||
| } |
There was a problem hiding this comment.
Can we remove this entire else statement, if not in use?
| } | ||
|
|
||
| } | ||
|
|
||
| // Cross-browser implementation of element.addEventListener() | ||
| function addPassiveWindowOnloadListener() { | ||
| window.addEventListener("load", function() { | ||
| CheckIfURLShouldBeBlocked(); | ||
| // XXX: Work around slow test startup. |
There was a problem hiding this comment.
suggestion: (non-blocking) Can we change this from XXX to TODO/FIXME?
Please note the part with "XXX: ..". I needed to add a timeout for Selenium to work, but when manually testing the same page (badges.html) it worked fine without the timeout. It seems that
browser.runtime.sendMessageis too early in some scenarios.fixes #601