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: inject polyfill earlier when world: MAIN is supported #606

Merged
merged 4 commits into from
Sep 17, 2024

Conversation

sidvishnoi
Copy link
Member

@sidvishnoi sidvishnoi commented Sep 17, 2024

Context

Fixes #590
Without needing to sacrifice Firefox support!

Changes proposed in this pull request

If browser supports world: MAIN for content script, we inject directly using scripting.registerContentScripts API (can detect WM support immediately on page load). If not, we inject like before from contentScript (slow, requires timeout for WM support detection).

We inject via scripting.registerContentScripts API and not directly via manifest.json, as we can catch the lack of support of world: MAIN via this API. With this detection, we can still support Firefox versions older than 128.

Update polyfill to support injecting twice - only the first injection works - as we detect if already injected.

Note: In content script, we inject polyfill from contentScript and not using scripting.executeScript in background as we'd otherwise need to inject it manually to each tab.

What next?

When Firefox 128 is old enough, we can remove injectPolyfill from background.ts as well as contentScript.ts and register it directly in manifest.json with world: MAIN. When we do that, we'd update browser_specific_settings.gecko.strict_min_version in manifest.json from 110 to 128, and also remove polyfill/* from web_accessible_resources. (#607)

Test results:

Browser WM detection on page load WM detection later Other JS Events
Firefox 126
Firefox 130
Chrome 110

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/scripting/ExecutionWorld#browser_compatibility

@github-actions github-actions bot added area: content Improvements or additions to extension content script area: background Improvements or additions to extension background script labels Sep 17, 2024
Copy link

github-actions bot commented Sep 17, 2024

Extension builds preview

Name Link
Latest commit 5196937
Latest job logs Run #10904420369
BadgeDownload
BadgeDownload

Copy link
Member Author

@sidvishnoi sidvishnoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test-e2e

Copy link
Member

@raducristianpopa raducristianpopa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timing issue for Firefox < 128 still exists right?

src/background/services/background.ts Outdated Show resolved Hide resolved
src/background/services/background.ts Outdated Show resolved Hide resolved
src/content/polyfill.ts Outdated Show resolved Hide resolved
@sidvishnoi
Copy link
Member Author

The timing issue for Firefox < 128 still exists right?

Yes, but it'll at least continue to work like before. We just won't need to set min supported version to 128 this way.

Copy link
Member

@raducristianpopa raducristianpopa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can still discuss in this week's call if we want to drop support for older Firefox versions.

@sidvishnoi sidvishnoi merged commit 2d811da into main Sep 17, 2024
9 checks passed
@sidvishnoi sidvishnoi deleted the faster-polyfill-injection branch September 17, 2024 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: background Improvements or additions to extension background script area: content Improvements or additions to extension content script
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] HTMLLinkElement.relList.supports('monetization') is false on page load
2 participants