-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add temporary incontext_eligible pixel #304
Conversation
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
@@ -109,6 +109,9 @@ class AndroidInterface extends InterfacePrototype { | |||
} | |||
}) | |||
} | |||
|
|||
/** Noop */ | |||
firePixel (_pixelParam) {} |
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.
We don't want to fire any js pixel on Android because of how the interfaces are exposed.
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.
@GioSensation Just confirming, we were previously send pixels that Android was ignoring, and now we're stopping sending the pixels at all?
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.
Previously we were not sending any pixel outside the pulldown UI, so nothing on Android. Since this new pixel is not fired from the UI, but from the detection logic, we need to ensure nothing happens accidentally on Android.
* @param {HTMLElement} el | ||
* @param {Event['type']} type | ||
* @param {() => void} fn | ||
* @param {AddEventListenerOptions} [opts] |
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.
Just added the opts
object so we can use this more flexibly as per the standard addEventListener
specs.
this.addListener(input, 'pointerdown', () => { | ||
this.device.firePixel({pixelName: 'incontext_eligible'}) | ||
}, {once: true}) | ||
} |
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.
Fired once per page. This is gated appropriately on the client side to keep the implementation simpler. It's a temporary pixel anyway.
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.
NAB: We do the rest of our listener adding in this.decorateInput
-- is there a reason we don't add this one there too?
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.
decorateInput
is rather convoluted and has a ton of stuff in it. Adding it here makes it easy to add/remove. Given that this is a temporary thing it makes sense to leave it here.
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.
LGTM
@@ -109,6 +109,9 @@ class AndroidInterface extends InterfacePrototype { | |||
} | |||
}) | |||
} | |||
|
|||
/** Noop */ | |||
firePixel (_pixelParam) {} |
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.
@GioSensation Just confirming, we were previously send pixels that Android was ignoring, and now we're stopping sending the pixels at all?
this.addListener(input, 'pointerdown', () => { | ||
this.device.firePixel({pixelName: 'incontext_eligible'}) | ||
}, {once: true}) | ||
} |
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.
NAB: We do the rest of our listener adding in this.decorateInput
-- is there a reason we don't add this one there too?
Task/Issue URL: https://app.asana.com/0/1204538113123459/1204538113123459 Autofill Release: https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/6.5.1 ## Description Updates Autofill to version [6.5.1](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/6.5.1). ### Autofill 6.5.1 release notes ## What's Changed * Disable pixel for all platforms but extension by @GioSensation in duckduckgo/duckduckgo-autofill#309 * Fix release script for BSK by @GioSensation in duckduckgo/duckduckgo-autofill#310 Included in 6.5.0: - Improve debugging by @GioSensation in duckduckgo/duckduckgo-autofill#293 - Reduce the calls on check position when there are many mutations by @alistairjcbrown in duckduckgo/duckduckgo-autofill#288 - Window release fetch tags before checkout by @alistairjcbrown in duckduckgo/duckduckgo-autofill#296 - Autofill compare generated test suites by @greyivy in duckduckgo/duckduckgo-autofill#283 - Update release scripts with latest Apple tooling by @GioSensation in duckduckgo/duckduckgo-autofill#299 - Add temporary incontext_eligible pixel by @GioSensation in duckduckgo/duckduckgo-autofill#304 - Fix double-click issue by @GioSensation in duckduckgo/duckduckgo-autofill#306 - Fix a whole bunch of existing failing test cases by @GioSensation in duckduckgo/duckduckgo-autofill#308 **Full Changelog**: duckduckgo/duckduckgo-autofill@6.5.0...6.5.1 ## Steps to test This release has been tested during autofill development. For smoke test steps see [this task](https://app.asana.com/0/1198964220583541/1200583647142330/f). Co-authored-by: GioSensation <GioSensation@users.noreply.github.com>
Reviewer: @alistairjcbrown
Asana: https://app.asana.com/0/0/1204486716187196/f
Description
Sends a pixel when an email-eligible field is first interacted with. Clients will add info on install time and decide if to send the pixel at all. This pixel is temporary and will be removed in a couple of weeks.
Steps to test
See duckduckgo/duckduckgo-privacy-extension#1943.