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

Fix custom Brave notification ads should be attached to the browser window #22203

Closed
btlechowski opened this issue Apr 11, 2022 · 5 comments · Fixed by brave/brave-core#17997
Closed

Comments

@btlechowski
Copy link

btlechowski commented Apr 11, 2022

Follow up to #19464.
On Windows the custom ad notification is not anchored to Brave Window so the z-ordering does not work, the ad stays on top.

Steps to Reproduce

  1. Clean install
  2. Run Brave with: --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_attach_ad_notification_to_browser_window/true"
  3. Enable rewards and ads
  4. Trigger a notification ad

Actual result:

Notification ad is not anchored to the browser window, the ad stays on top
19464_Windows_covered

Expected result:

Notification ad is anchored to the browser window

Reproduces how often:

Easily reproduced

Brave version (brave://version info)

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)

cc @brave/qa-team @jsecretan @tmancey @aseren

@tmancey tmancey added priority/P3 The next thing for us to work on. It'll ride the trains. QA/Yes release-notes/exclude and removed OS/Desktop labels Mar 1, 2023
@aseren aseren self-assigned this Apr 11, 2023
@tmancey tmancey changed the title Custom ad notifications is not attached to the browser window Fix custom Brave Ad notifications should be attached to the browser window Apr 13, 2023
@tmancey tmancey changed the title Fix custom Brave Ad notifications should be attached to the browser window Fix custom Brave notification ads should be attached to the browser window Apr 13, 2023
@brave-builds brave-builds added this to the 1.52.x - Nightly milestone Apr 14, 2023
@btlechowski
Copy link
Author

btlechowski commented May 23, 2023

Verification passed on

Brave 1.52.93 Chromium: 113.0.5672.92 (Official Build) beta (64-bit)
Revision b6f521170062a1fa8a82c33fb223b06fec566da1-refs/branch-heads/5672_63@{#10}
OS Ubuntu 18.04 LTS

Issue still reproducible. Per our process logged #30508


Verification passed on

Brave 1.52.109 Chromium: 114.0.5735.26 (Official Build) beta (64-bit)
Revision 7075cbb66f0542ac3e01ddfde6b813e7d61118a5-refs/branch-heads/5735@{#454}
OS Windows 10 Version 22H2 (Build 19045.2965)

Verified test plan from the description

22203 windows

@stephendonner
Copy link

stephendonner commented May 23, 2023

Verified PASSED using

Brave 1.52.109 Chromium: 114.0.5735.26 (Official Build) beta (x86_64)
Revision 7075cbb66f0542ac3e01ddfde6b813e7d61118a5-refs/branch-heads/5735@{#454}
OS macOS Version 11.7.7 (Build 20G1345)

ad-notification

@aseren
Copy link

aseren commented May 23, 2023

The parameter name was changed to should_attach_ad_notification_to_browser_window for Beta 1.52. For Nightly version it was renamed to use_same_z_order_as_browser_window and set to true by default: #30508 (comment)

@tmancey
Copy link
Contributor

tmancey commented May 23, 2023

I will raise an issue to add the latest feature/param/values to a README.md you can access in the repo.

@btlechowski
Copy link
Author

Thanks @aseren @tmancey . We will retest with the new parameter.

@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