[ads] Add search result ad clicked InfoBar #25393

aseren · 2024-08-30T16:57:12Z

InfoBar on Desktop:

InfoBar on iOS:

InfoBar on Android:

Resolves brave/brave-browser#39625

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

The functionality should be tested for iOS, Android and Desktop

Test case 1

Fresh install
Open staging search search.brave.software in a new tab
Trigger search result ad for a staging server with the giraffe request
Click search result ad
Confirm that Search Result Ad Clicked InfoBar was shown
Click Learn more
Make sure that Brave Search Result Ad help page is opened
Open staging search search.brave.software in a new tab
Trigger search result ad for a staging server with the giraffe request
Click search result ad
Confirm that Search Result Ad Clicked InfoBar was not shown for second time

Test case 2

Fresh install
Open staging search search.brave.software in a new tab
Trigger search result ad for a staging server with the giraffe request
Click search result ad
Confirm that Search Result Ad Clicked InfoBar was shown
Click a link in the page and navigate to a different page
Confirm that the Infobar disappears automatically
Open staging search search.brave.software in a new tab
Trigger search result ad for a staging server with the giraffe request
Click search result ad
Confirm that Search Result Ad Clicked InfoBar was not shown for second time

Test case 3

Fresh install
Open staging search search.brave.software in a new tab
Trigger search result ad for a staging server with the giraffe request
Click search result ad
Confirm that Search Result Ad Clicked InfoBar was shown
Type a new URL in the URL bar and navigate to a page with entered URL
Confirm that the Infobar disappears automatically
Open staging search search.brave.software in a new tab
Trigger search result ad for a staging server with the giraffe request
Click search result ad
Confirm that Search Result Ad Clicked InfoBar was not shown for second time

...r/brave_ads/creatives/search_result_ad/creative_search_result_ad_clicked_infobar_delegate.cc

...ts/brave_ads/content/browser/creatives/search_result_ad/creative_search_result_ad_handler.cc

tmancey

LGTM (see comments)

PrivacyMatters · 2024-09-06T16:44:23Z

Apologies if I missed a request for review.

I'd suggest we amend the notice slightly to read "Thank you for supporting Brave Search by clicking on a private ad! Unlike Big Tech, we measure ad performance anonymously and preserve your privacy. [Learn more / your opt-out choices]

We needs to link to clear simple info/guidance including choices available and how a user can delete data stored on their device. I know @fmarier has been working on this inc liaising with Brendan

aseren · 2024-09-06T17:27:21Z

@PrivacyMatters, suggested link page content is available in the spec. Would you able to check if it needs to be changed, please?
Data deletion functionality was implemented here: #24695. Thanks!

PrivacyMatters · 2024-09-06T17:41:58Z

@fmarier see #25393 (comment)

fmarier · 2024-09-06T20:23:02Z

@PrivacyMatters, suggested link page content is available in the spec. Would you able to check if it needs to be changed, please?

The text is not finalized yet (I will mention here once ready), but the current version is:

Thank you for supporting Brave Search by clicking on a private ad! Unlike Big Tech, we measure ad performance anonymously and preserve your privacy. [Learn more / opt out choices ]

This PR is also blocked on having a page to link to for the "Learn more" link above. It's coming soon.

iefremov

chromium_src lgtm

ios/brave-ios/Sources/Brave/Frontend/Browser/SearchResultAdClickedInfoBar.swift

ios/brave-ios/Sources/Brave/Frontend/Browser/Toast.swift

mkarolin

strings++

ShivanKaul

The proposed text and user experience looks fine (modulo the linked help page actually working)

PrivacyMatters · 2024-09-11T13:12:27Z

@fmarier @ShivanKaul if a user deletes the data stored on their device in clicking an ad I assume the next time they click an ad they will see the notice

aseren · 2024-09-11T17:09:34Z

@fmarier @ShivanKaul if a user deletes the data stored on their device in clicking an ad I assume the next time they click an ad they will see the notice

That's correct. After Brave Ads data deletion, user will get the notice again when they click an ad

aseren · 2024-09-11T18:29:14Z

ios/brave-ios/Sources/Brave/Frontend/Browser/SearchResultAdClickedInfoBar.swift

+    addSubview(toastView)
+    toastView.snp.makeConstraints { make in
+      make.left.right.height.equalTo(self)
+      self.animationConstraint = make.top.equalTo(self).offset(SimpleToastUX.toastHeight).constraint


SimpleToastUX.toastHeight is used for an animation constraint, because it is suitable for what we have for dismiss animation in the parent class here: https://github.com/brave/brave-core/blob/master/ios/brave-ios/Sources/Brave/Frontend/Browser/Toast.swift#L84

UIView.animate( withDuration: duration, animations: { self.animationConstraint?.update(offset: SimpleToastUX.toastHeight) self.layoutIfNeeded() }, ...

I think we're going to have issues with this though since this copy is multiline and will get translated to likely be much taller whereas other toasts typically are capped at a certain height.

Agreed, this affects starting/ending Toast heights during animation.
I will try to find a way of animation basing on the actual Toast height, not the predefined constants.

aseren · 2024-09-11T18:43:19Z

The text is not finalized yet (I will mention here once ready), but the current version is:

Thank you for supporting Brave Search by clicking on a private ad! Unlike Big Tech, we measure ad performance anonymously and preserve your privacy. [Learn more / opt out choices ]

The PR description is updated with screenshots with the new Infobar text.

fallaciousreasoning

Icons stuff LGTM - sorry don't know anything about iOS so I can't provide a more helpful review!

aseren · 2024-10-08T16:28:47Z

The current PR is blocked by brave/brave-browser#40952: we need brave://ads-internals to be implemented first.

fmarier · 2024-11-15T23:04:21Z

...r/brave_ads/creatives/search_result_ad/creative_search_result_ad_clicked_infobar_delegate.cc

+constexpr int kIconSize = 20;
+
+constexpr char kLearnMoreUrl[] =
+    "https://support.brave.com/hc/en-us/articles/360026361072-Brave-Ads-FAQ";


This will need to point to https://search.brave.com/help/conversion-reporting once it's live.

fmarier · 2024-11-15T23:06:52Z

ios/brave-ios/Sources/Brave/Frontend/Browser/SearchResultAdClickedInfoBar.swift

+  static let toastCloseButtonWidth: CGFloat = 20.0
+  static let toastLabelFont = UIFont.preferredFont(forTextStyle: .subheadline)
+  static let toastBackgroundColor = UIColor(braveSystemName: .schemesOnPrimaryFixed)
+  static let learnMoreUrl = "https://support.brave.com/hc/en-us/articles/360026361072-Brave-Ads-FAQ"


This one will also need to be https://search.brave.com/help/conversion-reporting

fmarier · 2024-11-15T23:11:36Z

There is another aspect to add the to test plan here:

Clear profile (to reset the new pref)
Visit Brave Search again and click on the ad
Confirm the infobar is present but don't touch it.
Click a link in the page and navigate to a different page.
Confirm that the infobar disappears automatically.
Repeat steps 1-5 but in Step 4 instead of clicking a link, type a new URL in the URL bar. That direct navigation should also make the infobar go away automatically.

aseren · 2024-11-20T01:54:58Z

There is another aspect to add the to test plan here:

Clear profile (to reset the new pref)

Visit Brave Search again and click on the ad

Confirm the infobar is present but don't touch it.

Click a link in the page and navigate to a different page.

Confirm that the infobar disappears automatically.

Repeat steps 1-5 but in Step 4 instead of clicking a link, type a new URL in the URL bar. That direct navigation should also make the infobar go away automatically.

Added 👍

aseren · 2024-11-20T19:59:49Z

browser/sources.gni

@@ -585,6 +585,7 @@ brave_chrome_browser_ui_allow_circular_includes_from +=
 brave_chrome_browser_allow_circular_includes_from = [
  "//brave/browser/ui",
  "//brave/browser/brave_ads:impl",
+  "//brave/browser/brave_ads/creatives/search_result_ad",


Allowed a circular dependency from search_result_ad target.

cc @bridiver

cdesouza-chromium · 2024-12-02T19:58:11Z

...r/brave_ads/creatives/search_result_ad/creative_search_result_ad_clicked_infobar_delegate.cc

+      CreateConfirmInfoBar(std::unique_ptr<ConfirmInfoBarDelegate>(
+          new CreativeSearchResultAdClickedInfoBarDelegate())));


Could this use std::make_unique?

Fixed, thanks!

cdesouza-chromium

LGTM

github-actions bot assigned aseren Aug 30, 2024

aseren force-pushed the issues/39625 branch 2 times, most recently from f681d88 to 8c14a7c Compare September 4, 2024 00:44

aseren changed the title ~~[ads] Add Brave Ads privacy notice infobar~~ [ads] Add search result ad clicked InfoBar Sep 4, 2024

aseren marked this pull request as ready for review September 4, 2024 01:24

aseren requested review from a team as code owners September 4, 2024 01:24

aseren force-pushed the issues/39625 branch from 8c14a7c to 22abcc5 Compare September 4, 2024 01:33

github-actions bot added the puLL-Merge label Sep 4, 2024

aseren force-pushed the issues/39625 branch from 22abcc5 to 28e0146 Compare September 4, 2024 01:36

tmancey reviewed Sep 4, 2024

View reviewed changes

...r/brave_ads/creatives/search_result_ad/creative_search_result_ad_clicked_infobar_delegate.cc Outdated Show resolved Hide resolved

tmancey reviewed Sep 4, 2024

View reviewed changes

...ts/brave_ads/content/browser/creatives/search_result_ad/creative_search_result_ad_handler.cc Outdated Show resolved Hide resolved

tmancey approved these changes Sep 4, 2024

View reviewed changes

aseren force-pushed the issues/39625 branch from 100fec3 to 9c839b3 Compare September 6, 2024 14:56

aseren requested a review from fallaciousreasoning September 6, 2024 15:17

diracdeltas requested review from fmarier, PrivacyMatters and ShivanKaul September 6, 2024 16:38

iefremov approved these changes Sep 6, 2024

View reviewed changes

kylehickinson requested changes Sep 9, 2024

View reviewed changes

mkarolin approved these changes Sep 9, 2024

View reviewed changes

ShivanKaul approved these changes Sep 10, 2024

View reviewed changes

aseren force-pushed the issues/39625 branch from 9c839b3 to c3fdf69 Compare September 10, 2024 21:02

aseren force-pushed the issues/39625 branch from c3fdf69 to 214ca81 Compare September 11, 2024 17:01

aseren commented Sep 11, 2024

View reviewed changes

aseren requested a review from kylehickinson September 11, 2024 18:29

aseren force-pushed the issues/39625 branch 2 times, most recently from 8eede73 to b4fe7f6 Compare September 12, 2024 19:48

fallaciousreasoning approved these changes Sep 13, 2024

View reviewed changes

aseren force-pushed the issues/39625 branch 2 times, most recently from 5869709 to 9a6c7c6 Compare November 15, 2024 20:18

fmarier requested changes Nov 15, 2024

View reviewed changes

aseren force-pushed the issues/39625 branch from 9a6c7c6 to a722194 Compare November 20, 2024 01:53

aseren requested review from fmarier and bridiver November 20, 2024 01:55

aseren commented Nov 20, 2024

View reviewed changes

aseren force-pushed the issues/39625 branch from a722194 to ccc51d9 Compare November 22, 2024 17:49

kylehickinson approved these changes Nov 22, 2024

View reviewed changes

[ads] Add search result ad clicked InfoBar

c7ecffe

aseren force-pushed the issues/39625 branch from ccc51d9 to 2261e4e Compare December 2, 2024 19:51

cdesouza-chromium reviewed Dec 2, 2024

View reviewed changes

Implement height-based animation for Toast on iOS

aec3d26

aseren force-pushed the issues/39625 branch from 2261e4e to aec3d26 Compare December 2, 2024 22:14

aseren requested a review from cdesouza-chromium December 2, 2024 22:15

cdesouza-chromium approved these changes Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ads] Add search result ad clicked InfoBar #25393

[ads] Add search result ad clicked InfoBar #25393

aseren commented Aug 30, 2024 •

edited

Loading

tmancey left a comment •

edited

Loading

PrivacyMatters commented Sep 6, 2024

aseren commented Sep 6, 2024

PrivacyMatters commented Sep 6, 2024

fmarier commented Sep 6, 2024

iefremov left a comment

mkarolin left a comment

ShivanKaul left a comment

PrivacyMatters commented Sep 11, 2024

aseren commented Sep 11, 2024 •

edited

Loading

aseren Sep 11, 2024 •

edited

Loading

kylehickinson Sep 13, 2024

aseren Sep 13, 2024 •

edited

Loading

aseren Nov 20, 2024

aseren commented Sep 11, 2024

fallaciousreasoning left a comment

aseren commented Oct 8, 2024

fmarier Nov 15, 2024

aseren Nov 20, 2024

fmarier Nov 15, 2024

aseren Nov 20, 2024

fmarier commented Nov 15, 2024

aseren commented Nov 20, 2024

aseren Nov 20, 2024 •

edited

Loading

cdesouza-chromium Dec 2, 2024

aseren Dec 2, 2024

cdesouza-chromium left a comment

		CreateConfirmInfoBar(std::unique_ptr<ConfirmInfoBarDelegate>(
		new CreativeSearchResultAdClickedInfoBarDelegate())));

[ads] Add search result ad clicked InfoBar #25393

Are you sure you want to change the base?

[ads] Add search result ad clicked InfoBar #25393

Conversation

aseren commented Aug 30, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

The functionality should be tested for iOS, Android and Desktop

Test case 1

Test case 2

Test case 3

tmancey left a comment • edited Loading

Choose a reason for hiding this comment

PrivacyMatters commented Sep 6, 2024

aseren commented Sep 6, 2024

PrivacyMatters commented Sep 6, 2024

fmarier commented Sep 6, 2024

iefremov left a comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

ShivanKaul left a comment

Choose a reason for hiding this comment

PrivacyMatters commented Sep 11, 2024

aseren commented Sep 11, 2024 • edited Loading

aseren Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aseren Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aseren commented Sep 11, 2024

fallaciousreasoning left a comment

Choose a reason for hiding this comment

aseren commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmarier commented Nov 15, 2024

aseren commented Nov 20, 2024

aseren Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdesouza-chromium left a comment

Choose a reason for hiding this comment

aseren commented Aug 30, 2024 •

edited

Loading

tmancey left a comment •

edited

Loading

aseren commented Sep 11, 2024 •

edited

Loading

aseren Sep 11, 2024 •

edited

Loading

aseren Sep 13, 2024 •

edited

Loading

aseren Nov 20, 2024 •

edited

Loading