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

Custom ad notifications should be attached to the browser window #19464

Closed
tmancey opened this issue Nov 15, 2021 · 3 comments · Fixed by brave/brave-core#12295
Closed

Custom ad notifications should be attached to the browser window #19464

tmancey opened this issue Nov 15, 2021 · 3 comments · Fixed by brave/brave-core#12295

Comments

@tmancey
Copy link
Contributor

tmancey commented Nov 15, 2021

Custom ad notifications should be attached to the browser window (z-order)

@tmancey tmancey self-assigned this Nov 15, 2021
@tmancey tmancey added the priority/P3 The next thing for us to work on. It'll ride the trains. label Nov 16, 2021
@aseren aseren self-assigned this Dec 8, 2021
@aseren
Copy link

aseren commented Dec 22, 2021

I was able to attach notification popup to browser window by setting parent member of views::Widget::InitParams structure here: https://github.com/brave/brave-core/blob/master/browser/ui/views/brave_ads/ad_notification_popup_widget.cc#L26

Passing browser window handler to AdNotificationPopupWidget::InitWidget function will be done separately in this issue: #17593. So change current issue to blocked until #17593 will be resolved.

@btlechowski
Copy link

btlechowski commented Apr 11, 2022

In current state the feature is not usable on Linux nor on Windows

Verification passed on

Brave 1.38.83 Chromium: 100.0.4896.79 (Official Build) beta (64-bit)
Revision 8fb749dcab8700c24213791969e59deb72fee36f-refs/branch-heads/4896@{#1015}
OS Ubuntu 18.04 LTS

Used command line:

--enable-logging=stderr --vmodule="*/variations/*"=6,"*/bat-native-ledger/*"=6,"*/brave_rewards/*"=6,"*/bat-native-ads/*"=6,"*/bat-native-confirmations/*"=6,"*/brave_ads/*"=9,"*/brave_user_model/*"=6,"*/bat_ads/*"=6 --brave-ads-staging --rewards=staging=true --variations-server-url=https://v --enable-features="CustomAdNotifications:should_attached_ad_notification_to_browser_window/true"

Verified custom ad notification is attached to Brave Window:
19464_Ubuntu_attached

Logged
#22205
#22206
#22216
#22218

Verification passed on

Brave 1.38.83 Chromium: 100.0.4896.79 (Official Build) beta (64-bit)
Revision 8fb749dcab8700c24213791969e59deb72fee36f-refs/branch-heads/4896@{#1015}
OS Windows 10 Version 21H2 (Build 19044.1586)

Used command line:

--enable-logging=stderr --vmodule="*/variations/*"=6,"*/bat-native-ledger/*"=6,"*/brave_rewards/*"=6,"*/bat-native-ads/*"=6,"*/bat-native-confirmations/*"=6,"*/brave_ads/*"=9,"*/brave_user_model/*"=6,"*/bat_ads/*"=6 --brave-ads-staging --rewards=staging=true --variations-server-url=https://v --enable-features="CustomAdNotifications:should_attached_ad_notification_to_browser_window/true"

Verified when minimizing and maximizing the browser window
19464_Windows_Minimized

The z-ordering does not work on Window, per our process logged a follow up:
#22203

@stephendonner
Copy link

stephendonner commented Apr 12, 2022

Verified PASSED using

Brave 1.38.90 Chromium: 100.0.4896.79 (Official Build) beta (x86_64)
Revision 8fb749dcab8700c24213791969e59deb72fee36f-refs/branch-heads/4896@{#1015}
OS macOS Version 11.6.5 (Build 20G527)

Commandline used was:

--enable-logging=stderr --vmodule="*/variations/*"=6,"*/bat-native-ledger/*"=6,"*/brave_rewards/*"=6,"*/bat-native-ads/*"=6,"*/bat-native-confirmations/*"=6,"*/brave_ads/*"=9,"*/brave_user_model/*"=6,"*/bat_ads/*"=6 --brave-ads-staging --rewards=staging=true --variations-server-url=https://v --enable-features="CustomAdNotifications:should_attached_ad_notification_to_browser_window/true"

Cases:

Confirmed ads were "attached" to the browser window
Confirmed I could drag to reposition the ads
Confirmed repositioned ads were still loosely "attached" to the browser window, and moved along with it

light, attached light, repositioned dark, attached dark, repositioned
Screen Shot 2022-04-12 at 12 48 11 PM Screen Shot 2022-04-12 at 12 48 28 PM Screen Shot 2022-04-12 at 1 14 15 PM Screen Shot 2022-04-12 at 1 14 32 PM

Confirmed ads were correctly restored (parented) after minimizing their attached window

ad/window before minimize minimized restored window
Screen Shot 2022-04-13 at 4 45 50 PM Screen Shot 2022-04-13 at 4 45 58 PM Screen Shot 2022-04-13 at 4 46 06 PM

Encountered @btlechowski's already-filed #22203; couldn't cover the custom-ad notification with another app

example example
Screen Shot 2022-04-13 at 4 06 03 PM Screen Shot 2022-04-13 at 4 06 10 PM

@tmancey tmancey added this to Ads Jun 10, 2024
@tmancey tmancey moved this to Done in Ads Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment