-
Notifications
You must be signed in to change notification settings - Fork 46.8k
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
Fixes privacy measures issue in Chrome 101+ #26984
Conversation
Hi @joshuagornall! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
const userAgentData = await navigator.userAgentData.getHighEntropyValues([ | ||
'brands', | ||
]); |
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 navigator.userAgentData
supported by Firefox?
https://developer.mozilla.org/en-US/docs/Web/API/Navigator/userAgentData#browser_compatibility
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 now, we've added a fallback to use navigator.userAgent in browsers that don't support navigator.userAgentData.
Fall back to the old method if navigator.userAgentData isn't supported
IS_EDGE = navigator.userAgent.indexOf('Edg') >= 0; | ||
IS_FIREFOX = navigator.userAgent.indexOf('Firefox') >= 0; | ||
IS_CHROME = IS_EDGE === false && IS_FIREFOX === false; |
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.
Please run yarn prettier-all
to fix all linter issues
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 specific security reasons, I cannot pull local now and run terminal commands on the code. Last changes were directly through Github. If it is possible you can run yarn prettier-all and suggest changes I would be grateful. Thanks for your time Ruslan.
Co-authored-by: Ruslan Lesiutin <rdlesyutin@gmail.com>
Co-authored-by: Ruslan Lesiutin <rdlesyutin@gmail.com>
Co-authored-by: Ruslan Lesiutin <rdlesyutin@gmail.com>
break; | ||
} | ||
}); | ||
} else { |
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.
} else { | |
} else { |
navigator.userAgentData.brands.forEach(item => { | ||
switch (item.brand.toLowerCase()) { | ||
case 'google chrome': | ||
IS_CHROME = true; | ||
break; | ||
case 'firefox': | ||
IS_FIREFOX = true; | ||
break; | ||
case 'edge': | ||
IS_EDGE = true; | ||
break; | ||
} | ||
}); |
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.
navigator.userAgentData.brands.forEach(item => { | |
switch (item.brand.toLowerCase()) { | |
case 'google chrome': | |
IS_CHROME = true; | |
break; | |
case 'firefox': | |
IS_FIREFOX = true; | |
break; | |
case 'edge': | |
IS_EDGE = true; | |
break; | |
} | |
}); | |
const brandItems = navigator.userAgentData.brands; | |
IS_EDGE = brandItems.some(item => item.brand === 'Microsoft Edge'); | |
IS_FIREFOX = brandItems.some(item => item.brand === 'Mozilla Firefox'); | |
IS_CHROME = brandItems.some(item => item.brand === 'Google Chrome'); |
Lets put it this way, if there will be some weird custom user agents. This also changes string for Edge to Microsoft Edge
:
hey @hoxyq, coming back to this now. |
Closing as its been shipped in #27516. Thanks for working on this! |
Summary
Motivation:
The changes proposed in this pull request are designed to address upcoming changes in the Chrome browser. Starting with Chrome 101, the use of navigator.userAgent, navigator.appVersion, and navigator.platform are being discouraged and are subject to future deprecation. These properties are currently used in our codebase to determine the user's browser type and theme.
The pull request aims to modernize the codebase and keep it up-to-date with recent browser practices, ensuring its compatibility with newer versions of Chrome while maintaining its functionality in other major browsers.
Problem solved:
The pull request solves the problem of potential incompatibility and reduced functionality in future Chrome versions due to the aforementioned deprecation. If we continued using these properties, from Chrome 101 onward, our code could fail to accurately detect the user's browser type and theme.
This code checks the user agent string of the user's browser to determine whether they're using Chrome, Firefox, or Edge. It uses navigator.userAgent and searches for the presence of 'Edge', 'Firefox', 'Chrome'.
The getBrowserName function returns the browser name based on the checks made earlier, and getBrowserTheme returns whether the user has chosen a dark or light theme in their browser dev tools.
How did you test this change?
Testing Steps:
Automated Unit Testing:
Automated tests were added to verify the function getBrowserName returns the correct browser name for different user agent data. Mocking was used to simulate different browsers' user agent data. This way, we can automatically and repeatedly test our code against many different scenarios.
Manual Testing:
The application was manually tested on different browsers including Chrome, Firefox, and Edge. This manual testing is important to ensure that the function behaves as expected in real-world scenarios. I verified that the function correctly identified the browser name and theme in each case.
Browser Version Testing:
Testing was conducted on different versions of Chrome, specifically versions below 101 and versions 101 and above. For Chrome versions 101 and above, navigator.userAgentData was available and was used correctly to identify the browser type. For versions below 101, I ensured the application behaves as expected and doesn't throw any errors related to navigator.userAgentData not being available.
Testing Theme Detection:
For each browser, both the 'light' and 'dark' themes were tested to ensure the function getBrowserTheme returned the correct theme.
Verifying the solution:
The output from the getBrowserName and getBrowserTheme functions was compared against the expected values for each scenario. This allowed us to verify that the PR solves the issue it set out to address, i.e., correctly identifying the browser type and theme in light of the upcoming deprecation in Chrome 101.