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 no-op check to work with Safari and support of old browsers #582

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

xeenon
Copy link
Contributor

@xeenon xeenon commented Mar 7, 2024

The browser object is not a direct descendant of Object.prototype in Safari. This check should use instanceof for a more compatible check.

@xeenon
Copy link
Contributor Author

xeenon commented Mar 7, 2024

@diox @rpl @willdurand @Rob--W

@Rob--W
Copy link
Member

Rob--W commented Mar 8, 2024

The reason for looking up the immediate prototype is to avoid activating the polyfill when there is an element on the page that has an ID. Here is the PR that added the logic for more context: #153 (originally checking instanceof Node, but changed in response to #153 (comment) ).

What is the actual prototype chain in Safari? Is it expected to be somewhat stable?

@xeenon
Copy link
Contributor Author

xeenon commented Mar 9, 2024

What is the actual prototype chain in Safari? Is it expected to be somewhat stable?

The problem is the prototype is not reachable, so it can't be compared with the result of Object.getPrototypeOf().

I think a better approach would be to do a similar optional chain check looking for !globalThis.browser?.runtime?.id.

Verify the object that might be at `globalThis.browser` is not already
implementing the basic Web Extension APIs. This is symmetric with the
check to verify the poly fill is only included in extension contexts.

Don't use optional-chaining for these checks, since this needs to
continuing working in older browsers.
@xeenon xeenon changed the title Fix no-op check to work with Safari (#364) Fix no-op check to work with Safari and support of old browsers Mar 25, 2024
@Rob--W Rob--W merged commit 871b49d into mozilla:master Mar 28, 2024
1 check failed
@Rob--W
Copy link
Member

Rob--W commented Mar 28, 2024

Thanks for the patch @xeenon! We'll fix our CI and then publish a new version.

@Rob--W
Copy link
Member

Rob--W commented Apr 3, 2024

We still need to publish an update FYI.

rpl added a commit that referenced this pull request Apr 11, 2024
rpl added a commit that referenced this pull request Apr 11, 2024
…config (#584)

* chore(circleci): updated browser-tools orb to 1.4.8 and cimg/node to 20.12

* chore(deps-dev): bump chromedriver from 112.0.0 to 123.0.3

* chore(deps-dev): bump geckodriver from 3.2.0 to 4.3.3

* test: Update unit test to match new expected behaviors with changes applied from PR #582

---------

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@Rob--W
Copy link
Member

Rob--W commented Apr 16, 2024

FYI just released 0.11.0 that includes this patch: https://github.com/mozilla/webextension-polyfill/releases/tag/0.11.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants