Remove noatb.css. (#2312)

* Remove noatb.css. We're not using this CSS rule for extension detection anymore. Removing it fixes a race condition on extension install in Firefox. * Move onInstall check of ddg tabs as early as possible.
duckduckgo · Oct 31, 2023 · b8e4b9a · b8e4b9a
1 parent 0f6cb12
commit b8e4b9a
Show file tree

Hide file tree

Showing 10 changed files with 12 additions and 57 deletions.
diff --git a/Makefile b/Makefile
@@ -261,7 +261,7 @@ BUILD_TARGETS += $(BUILD_DIR)/public/js/inject.js
 ## SASS
 SASS = node_modules/.bin/sass
 SCSS_SOURCE = $(shell find shared/scss/ -type f)
-OUTPUT_CSS_FILES = $(BUILD_DIR)/public/css/noatb.css $(BUILD_DIR)/public/css/options.css $(BUILD_DIR)/public/css/feedback.css
+OUTPUT_CSS_FILES = $(BUILD_DIR)/public/css/options.css $(BUILD_DIR)/public/css/feedback.css
 $(BUILD_DIR)/public/css/base.css: shared/scss/base/base.scss $(SCSS_SOURCE)
 	$(SASS) $< $@
 $(BUILD_DIR)/public/css/%.css: shared/scss/%.scss $(SCSS_SOURCE)

diff --git a/browsers/chrome-mv3/manifest.json b/browsers/chrome-mv3/manifest.json
@@ -37,21 +37,6 @@
         "extension_pages": "script-src 'self'; object-src 'self'; frame-ancestors https://duckduckgo.com https://*.duckduckgo.com"
     },
     "content_scripts": [
-        {
-            "matches": [
-                "<all_urls>"
-            ],
-            "exclude_matches": [
-                "*://localhost/*",
-                "*://*.localhost/*"
-            ],
-            "match_about_blank": true,
-            "all_frames": true,
-            "css": [
-                "public/css/noatb.css"
-            ],
-            "run_at": "document_start"
-        },
         {
             "js": [
                 "public/js/content-scripts/autofill.js"

diff --git a/browsers/chrome/manifest.json b/browsers/chrome/manifest.json
@@ -47,9 +47,6 @@
             ],
             "match_about_blank": true,
             "all_frames": true,
-            "css": [
-                "public/css/noatb.css"
-            ],
             "js": [
                 "public/js/inject.js"
             ],

diff --git a/browsers/firefox/manifest.json b/browsers/firefox/manifest.json
@@ -48,9 +48,6 @@
             ],
             "match_about_blank": true,
             "all_frames": true,
-            "css": [
-                "public/css/noatb.css"
-            ],
             "js": [
                 "public/js/inject.js"
             ],

diff --git a/integration-test/atb.spec.js b/integration-test/atb.spec.js
@@ -52,7 +52,7 @@ test.describe('install workflow', () => {
             })
 
             // try get ATB params
-            await backgroundPage.evaluate(() => globalThis.dbg.atb.updateATBValues())
+            await backgroundPage.evaluate(async () => globalThis.dbg.atb.updateATBValues(await globalThis.dbg.Wrapper.getDDGTabUrls()))
 
             // wait for an exti call
             // eslint-disable-next-line no-unmodified-loop-condition
@@ -87,7 +87,7 @@ test.describe('install workflow', () => {
             await page.goto('https://duckduckgo.com/?natb=v123-4ab&cp=atbhc', { waitUntil: 'networkidle' })
 
             // try get ATB params again
-            await backgroundPage.evaluate(() => globalThis.dbg.atb.updateATBValues())
+            await backgroundPage.evaluate(async () => globalThis.dbg.atb.updateATBValues(await globalThis.dbg.Wrapper.getDDGTabUrls()))
             // eslint-disable-next-line no-unmodified-loop-condition
             while (numExtiCalled < 0) {
                 page.waitForTimeout(100)

diff --git a/shared/js/background/atb.js b/shared/js/background/atb.js
@@ -157,15 +157,14 @@ const ATB = (() => {
             return new URLSearchParams()
         },
 
-        updateATBValues: () => {
+        updateATBValues: (ddgTabUrls) => {
             // wait until settings is ready to try and get atb from the page
             return settings.ready()
                 .then(ATB.setInitialVersions)
-                .then(browserWrapper.getDDGTabUrls)
-                .then((urls) => {
+                .then(() => {
                     let atb
                     let params
-                    urls.some(url => {
+                    ddgTabUrls.some(url => {
                         params = ATB.getAcceptedParamsFromURL(url)
                         atb = params.has('atb') && params.get('atb')
                         return !!atb

diff --git a/shared/js/background/events.js b/shared/js/background/events.js
@@ -35,13 +35,15 @@ async function onInstalled (details) {
     tdsStorage.initOnInstall()
 
     if (details.reason.match(/install/)) {
+        // get tab URLs immediately to prevent race with install page
+        const ddgTabUrls = await browserWrapper.getDDGTabUrls()
         await settings.ready()
         settings.updateSetting('showWelcomeBanner', true)
         if (browserName === 'chrome') {
             settings.updateSetting('showCounterMessaging', true)
             settings.updateSetting('shouldFireIncontextEligibilityPixel', true)
         }
-        await ATB.updateATBValues()
+        await ATB.updateATBValues(ddgTabUrls)
         await ATB.openPostInstallPage()
 
         if (browserName === 'chrome') {

diff --git a/shared/js/background/wrapper.js b/shared/js/background/wrapper.js
@@ -85,14 +85,6 @@ export function mergeSavedSettings (settings, results) {
 
 export async function getDDGTabUrls () {
     const tabs = await browser.tabs.query({ url: 'https://*.duckduckgo.com/*' }) || []
-
-    tabs.forEach(tab => {
-        insertCSS({
-            target: { tabId: tab.id },
-            files: ['/public/css/noatb.css']
-        })
-    })
-
     return tabs.map(tab => tab.url)
 }
 

diff --git a/shared/scss/noatb.scss b/shared/scss/noatb.scss
diff --git a/unit-test/background/atb.js b/unit-test/background/atb.js
@@ -249,37 +249,23 @@ describe('complex install workflow cases', () => {
     })
 
     it('should handle the install process correctly if there\'s no DDG pages open', () => {
-        spyOn(browser.tabs, 'query').and.returnValue(Promise.resolve([]))
-
-        return atb.updateATBValues()
+        return atb.updateATBValues([])
             .then(() => {
                 validateExtiWasHit('v112-2')
                 expect(settings.getSetting('atb')).toEqual('v112-2')
                 expect(settings.getSetting('set_atb')).toEqual('v112-2')
             })
     })
     it('should handle the install process correctly if there\'s DDG pages open that pass an ATB param', () => {
-        // pretend one of the pages has an ATB to pass
-        spyOn(browser.tabs, 'query').and.returnValue([
-            { url: 'https://duckduckgo.com/about' },
-            { url: 'https://duckduckgo.com/?natb=v112-2ab' }
-        ])
-
-        return atb.updateATBValues()
+        return atb.updateATBValues(['https://duckduckgo.com/about', 'https://duckduckgo.com/?natb=v112-2ab'])
             .then(() => {
                 validateExtiWasHit('v112-2ab')
                 expect(settings.getSetting('atb')).toEqual('v112-2ab')
                 expect(settings.getSetting('set_atb')).toEqual('v112-2ab')
             })
     })
     it('should handle the install process correctly if there\'s DDG pages open that do not pass an ATB param', () => {
-        // pretend no pages have ATB to pass
-        spyOn(browser.tabs, 'query').and.returnValue([
-            { url: 'https://duckduckgo.com/about' },
-            { url: 'https://duckduckgo.com/?q=test' }
-        ])
-
-        return atb.updateATBValues()
+        return atb.updateATBValues(['https://duckduckgo.com/about', 'https://duckduckgo.com/?q=test'])
             .then(() => {
                 validateExtiWasHit('v112-2')
                 expect(settings.getSetting('atb')).toEqual('v112-2')