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

Should set as pinned state only when it's actually pinned #26301

Closed
MadhaviSeelam opened this issue Oct 26, 2022 · 13 comments · Fixed by brave/brave-core#23254
Closed

Should set as pinned state only when it's actually pinned #26301

MadhaviSeelam opened this issue Oct 26, 2022 · 13 comments · Fixed by brave/brave-core#23254

Comments

@MadhaviSeelam
Copy link

MadhaviSeelam commented Oct 26, 2022

Description

Steps to Reproduce

  1. Install 1.46
  2. launch Brave
  3. Click Pin button
  4. Setting should not changed to pinned
  5. Allow from os notification (Allow should be done within 20s after clicking pinned)
  6. Settings should be changed to pinned

Actual result:

Expected result:

Reproduces how often:

Easily

Brave version (brave://version info)

Brave 1.46.80 Chromium: 107.0.5304.68 (Official Build) beta (64-bit)
Revision a4e93e89d3b3df1be22214603fba846ad0183ca5-refs/branch-heads/5304@{#991}
OS Windows 11 Version 21H2 (Build 22000.1098)

Version/Channel Information:

  • Can you reproduce this issue with the current release? N/A
  • Can you reproduce this issue with the beta channel? Yes
  • Can you reproduce this issue with the nightly channel? Yes

Other Additional Information:

  • Does the issue resolve itself when disabling Brave Shields?
  • Does the issue resolve itself when disabling Brave Rewards?
  • Is the issue reproducible on the latest version of Chrome?

Miscellaneous Information:

@simonhong @rebron

@MadhaviSeelam MadhaviSeelam changed the title Brave browser is not pinned to taskbar when clicked on May be later on the first run dialog Brave browser is not pinned to taskbar when Pin to taskbar is checked the first run dialog Oct 26, 2022
@MadhaviSeelam MadhaviSeelam changed the title Brave browser is not pinned to taskbar when Pin to taskbar is checked the first run dialog Brave browser is not pinned to taskbar when Pin to taskbar is checked in the first run dialog Oct 26, 2022
@simonhong
Copy link
Member

simonhong commented Oct 26, 2022

Pin state is expected behavior now. Checkbox is only in effective when user clicks Set Brave as.. button.
@rebron Should we pin always when user checks?

Regarding to default browser state, Browser should not be set as a default when user clicks Maybe later.
I tested and can't repro. @MadhaviSeelam Can you check again? Maybe it's set as a default already?

@MadhaviSeelam
Copy link
Author

MadhaviSeelam commented Oct 27, 2022

@simonhong - so this might be happening with a fresh install
Scenario 1: Fresh installs

  • Install 1.46.80
  • launch Brave
  • either select not select Pin to taskbar in the first run dialog
  • Click May be later
  • go to brave://settings/getStarted

image

Scenario 2: Pin Brave to taskbar

  • new profile
  • already Brave is pinned to taskbar
  • launch Brave
  • Click May be later
  • go to brave://settings/getStarted

image

@MadhaviSeelam
Copy link
Author

Reproduced this issue still after double notification of Pin to taskbar issue fix in #36470

App notifications in OS settings turned on

notifications.turned.on.mp4

App notifications in OS settings turned off

app.notification.turned.off.mp4

@mherrmann
Copy link

During my recent work on #36470, I don't think I ever got the initial popup by Brave that has the checkbox for pinning to the taskbar:

image

Maybe it only appears on older Windows versions than the one I have.

I did get the OS-level notifications.

image

The OS-level notifications come from upstream.

One thing I saw in the code is that we have our own logic for pinning, in brave_shell_integration_win.cc. It felt strange to me that we have such code when similar code exists upstream.

My suspicion is that the problem can be fixed by invoking upstream's implementation for pinning to the task bar. And that we should clean up our associated code if it is no longer used. To that end, it would be interesting to find out if the initial run popup still gets shown on some Windows versions.

@simonhong
Copy link
Member

simonhong commented Apr 17, 2024

We've not been using first-run dialog for a long time as we prefer welcome page flow to first run dialog.
So, this os task bar notification will not be shown by first run dialog anymore.
However, we still have Pin settings at brave://settings/getStarted
It seems Windows want to double check whether user want to pin or not by using that os notification.
If user doesn't click Yes, it's still not pinned but settings state becomes pinned because we have assumption that
pin request will be done w/o more user action. but it's changed in Windows.
Although, settings page has proper state after reloading, I think we need to find more better way to make it pinned w/o showing os notiification if possible. Or should update settings page after getting final pin state.

I could see this os notification when I try to pin from firefox. Maybe it's triggered by Win api.

@simonhong
Copy link
Member

Checking

@simonhong simonhong self-assigned this Apr 22, 2024
@simonhong
Copy link
Member

simonhong commented Apr 22, 2024

It's very difficult to debug as pin/unpin Win api is undocumented api.
At first, it worked as expected w/o user consent but it seems changed.
To pin brave to task bar, user should agree via os notification.
Not sure what we can do here.
Should we change UX or settings text such as Ask to Windows to pin to task bar and sub label like To pin to task bar, user should allow from os notification. WDYT? @rebron @aguscruiz

image

@aguscruiz
Copy link

How about something like:

Title: Pin to task bar
Description: Click Pin and confirm in the following Windows notification.

@rebron @sangwoo108 ?

@simonhong
Copy link
Member

How about something like:

Title: Pin to task bar Description: Click Pin and confirm in the following Windows notification.

@rebron @sangwoo108 ?

I'm fine with it

@simonhong simonhong changed the title Brave browser is not pinned to taskbar when Pin to taskbar is checked in the first run dialog Should set as pinned state only when it's actually pinned Apr 24, 2024
@simonhong
Copy link
Member

Updated this issue's title and description as we don't use first run dialog.

simonhong added a commit to brave/brave-core that referenced this issue Apr 24, 2024
fix brave/brave-browser#26301

Polling the pinned state after asking to Windows because it's actually pinned
when user allowed via os notification.
We'll check 10 times with 2s interval after asking to Windows.
If user doesn't react or disallowed within that time, we doesn't change the state.
simonhong added a commit to brave/brave-core that referenced this issue Apr 24, 2024
* Update short cut pin state properly on Windows

fix brave/brave-browser#26301

Polling the pinned state after asking to Windows because it's actually pinned
when user allowed via os notification.
We'll check 10 times with 2s interval after asking to Windows.
If user doesn't react or disallowed within that time, we doesn't change the state.
@github-project-automation github-project-automation bot moved this from Todo to Done in Desktop Papercuts Apr 24, 2024
@brave-builds brave-builds added this to the 1.67.x - Nightly milestone Apr 24, 2024
@MadhaviSeelam
Copy link
Author

MadhaviSeelam commented May 2, 2024

Verification PASSED using

Brave | 1.67.59 Chromium: 124.0.6367.118 (Official Build) nightly (64-bit)
-- | --
Revision | 96b3194901e5b7509e4e2d7408e73b67c552aa77
OS | Windows 11 Version 23H2 (Build 22631.3527)

New Install*

Case 1: Pin Brave to taskbar via first notification during initial install - PASSED

  1. Installed 1.67.59
  2. launched Brave
  3. clicked Yes on Would you like to pin Brave Nightly to your taskbar?
  4. clicked context menu on taskbar Brave icon
  5. opened brave://settings/getStarted in a new tab

Confirmed Brave is pinned to taskbar

Confirmed Brave is already pinned text shown in brave://settings/getStarted

step 3 step 4 step 5
image image image

Case 2: Do not Pin Brave to taskbar during initial install but Pin via brave://settings/getStarted - PASSED

  1. Unpin Brave if it's pinned
  2. Installed 1.67.59
  3. launched Brave
  4. clicked No on Would you like to pin Brave Nightly to your taskbar?
  5. clicked context menu on taskbar Brave icon
  6. verified Brave is not pinned to taskbar
  7. opened brave://settings/getStarted in a new tab
  8. clicked Pin button
  9. clicked Yes on the OS notification Would you like to pin Profile 1 - Brave to your taskbar?
  10. verified Pin button is dismissed

Confirmed Brave is pinned to taskbar

Confirmed Brave is already pinned text shown in brave://settings/getStarted

step 3 step 5 step 7 step 8 step 9
image image image image image

Case 3: Dismiss initial OS notification and Pin Brave to taskbar via brave://settings/getStarted - PASSED

  1. Unpin Brave if it's pinned
  2. Installed 1.67.59
  3. launched Brave
  4. ignore or do not click anything on the OS notification Would you like to pin Brave Nightly to your taskbar?
  5. clicked context menu on taskbar Brave icon
  6. verified Brave is not pinned to taskbar
  7. opened brave://settings/getStarted in a new tab
  8. clicked Pin button
  9. clicked Yes on the OS notification Would you like to pin Profile 1 - Brave to your taskbar?
  10. verified Pin button is dismissed
  11. click x on first notification

Confirmed Brave is pinned to taskbar

Confirmed Brave is already pinned text shown in brave://settings/getStarted

example example example example
image image image image

Case 4: Pin to taskbar via Brave icon context menu - PASSED

  1. Unpin Brave if it's pinned
  2. Installed 1.67.59
  3. launched Brave
  4. verified OS notification Would you like to pin Brave Nightly to your taskbar? is shown
  5. clicked context menu on taskbar Brave icon
  6. clicked Brave icon context menu in the taskbar
  7. clicked Pin to taskbar
  8. clicked x on the os notification
  9. opened brave://settings/getStarted in a new tab

Confirmed Brave is pinned to taskbar

Confirmed Pin button is dismissed in brave://settings/getStarted and Brave is already pinned text shown

example example example
image image image

New Profile

Case 5: Pin Brave to taskbar via first OS notification - PASSED

  1. Unpin existing profile on the taskbar
  2. new profile
  3. Launch brave
  4. confirmed Would you like to pin Brave Nightly to your taskbar? notification is shown
  5. clicked Yes on the notification
  6. opened brave://settings/getStarted in a new tab

Confirmed setting is changed to pinned state and the text is changed to Brave is already pinned

example example example
image image image

Case 6: Do not Pin Brave to taskbar during initial install but Pin via brave://settings/getStarted - PASSED

  1. Unpin existing profile on the taskbar
  2. new profile
  3. Launch brave
  4. clicked x on the first notification
  5. opened brave://settings/getStarted in a new tab
  6. Clicked Pin button
  7. confirmed `Would you like to pin Profile 1 - Brave to your taskbar? is shown
  8. clicked Yes on first notification
  9. confirmed setting is changed to pinned state and the text is changed to Brave is already pinned
example example example
image image image

@kjozwiak
Copy link
Member

kjozwiak commented May 8, 2024

The above requires 1.66.103 or higher for 1.66.x verification 👍 Removing the QA Pass label as we'll need to re-verify using 1.66.x. Used brave/brave-core#23254 (comment) & #26301 (comment) as the needed verification on Nightly as per brave/brave-core#23490 (review).

@MadhaviSeelam
Copy link
Author

Verification PASSED using

Brave | 1.66.104 Chromium: 125.0.6422.41 (Official Build) (64-bit)
-- | --
Revision | 6213f85e37bb676772fb221ce11a41c2b1be0fe2
OS | Windows 11 Version 23H2 (Build 22631.3527)

Case 1: Pin Brave to taskbar via brave://settings/getStarted - PASSED

  1. Installed 1.66.104
  2. launched Brave
  3. clicked context menu on taskbar Brave icon
  4. verified Brave is not pinned to taskbar
  5. opened brave://settings/getStarted in a new tab
  6. clicked Pin button
  7. clicked Yes on the OS notification Would you like to pin Profile 1 - Brave to your taskbar?
  8. verified Pin button is dismissed in brave://settings/getStarted page
  9. clicked context menu on taskbar Brave icon

Confirmed Brave is pinned to taskbar

Confirmed Brave is already pinned text shown in brave://settings/getStarted

step 4 step 5 step 7 step 8 step 9
image image image image image

Case 2: Pin to taskbar via Brave icon context menu - PASSED

  1. Unpin Brave if it's pinned
  2. Installed 1.66.104
  3. launched Brave
  4. clicked context menu on taskbar Brave icon
  5. verified Brave is not pinned to taskbar
  6. clicked Brave icon context menu in the taskbar
  7. clicked Pin to taskbar
  8. opened brave://settings/getStarted in a new tab

Confirmed Brave is pinned to taskbar

Confirmed Pin button is dismissed in brave://settings/getStarted and Brave is already pinned text shown

step 5 step 7 step 8
image image image

Case 3: Do not pin Brave via OS notification - PASSED

  1. Unpin Brave if it's pinned
  2. Installed 1.66.104
  3. launched Brave
  4. clicked context menu on taskbar Brave icon
  5. verified Brave is not pinned to taskbar
  6. opened brave://settings/getStarted in a new tab
  7. clicked Pin button
  8. clicked No, thanks on the OS notification Would you like to pin Profile 1 - Brave to your taskbar?
  9. verified Pin button is still shown in brave://settings/getStarted page
  10. clicked context menu on taskbar Brave icon

Confirmed Brave is not pinned to taskbar

Confirmed Pin button is not dismissed in brave://settings/getStarted

step 4 step 7 step 9
image image image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

8 participants