-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add a data collection consent example #572
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
Conversation
@wagnerand, I can't assign you as a reviewer but please review |
The demo bundles DOMPurify, which is a complex library that occasionally needs an update. That could cause friction at submission time if the library is outdated or somehow changed during minification. In the interest of having a minimal example, is it possible to have the functionality without DOMPurify? |
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.
@dotproto added some initial comments on the texts and comments
"message": "Data Collection Consent Dialog Example" | ||
}, | ||
"consentPersonalIntroHTML": { | ||
"description": "A statement notify the user that the extension wants to access information that could be used to personally identify them.", |
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.
"description": "A statement notify the user that the extension wants to access information that could be used to personally identify them.", | |
"description": "A statement notifying the user that the extension wants to access information that could be used to identify them.", |
}, | ||
"consentPersonalIntroHTML": { | ||
"description": "A statement notify the user that the extension wants to access information that could be used to personally identify them.", | ||
"message": "Can we collect the following <strong>data that personally identifies you:</strong>" |
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.
"message": "Can we collect the following <strong>data that personally identifies you:</strong>" | |
"message": "Can we collect this <strong>data that identifies you</strong>:" |
/* exported consent */ | ||
// eslint-disable-next-line no-unused-vars | ||
var consent = (function() { | ||
// This library is wrapped in an immediately invoked function expression |
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.
Could we add a holistic description of this function here or in the readme?
const noop = () => {}; | ||
|
||
/** | ||
* A promisified version of setTimeout with AbortController support |
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.
Could we add a comment about the purpose of this code otherwise, it takes quite a while to get to the comments that explain that
const browser = globalThis.browser ?? chrome; | ||
|
||
/** | ||
* A "no operation" function. It doesn't do anything. |
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.
Why is it here then?
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.
Not sure if this is a rhetorical question. Just in case, noop functions are useful in situations where a function is called unconditionally, but you don't want that operation to have any side effects (see NOP (code)).
In this case I think we're probably better off just dropping it entirely. I'll do that in a follow-up commit.
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.
@dotproto, not entirely. We promote the extension examples as part of our get-started flow. Therefore, it's entirely possible that someone who visits here is not particularly skilled in JavaScript development and so may be bemused as to why you have something that does nothing.
Sure, we just need to decide now to revise the example to avoid the current uses of HTML. We can replace the |
Co-authored-by: rebloor <[email protected]>
I don't think this example is very helpful in its current state. The code seems overly complicated and focuses on an implementation that in many cases can't be used like that in real extensions. Where developers struggle the most is the UX itself, we should focus the example on that, potentially outside of this repository. |
Closing this PR as I'm not actively working on improving the example. I'd like to revisit the idea of providing a reference example for a consent experience in the future. |
Description
This PR introduces a new example that demonstrates how an extension developer could implement a extension that is compliant with Add-ons Policies. The consent UI in the example is modeled after the UI shown in the "Combination: Personal and technical data" section of the Prompt users for data and privacy consents page.
Motivation
It can be difficult for developers to understand how to implement a compliant data collection consent experience in their extensions. This PR aims to ease that burden by providing a concrete example of a compliant implementation. Developers could either use this example as the basis for their implementation or use this as a functional example to compare against their own.
Additional details
N/A
Related issues and pull requests
N/A