Closed Bug 1358363 Opened 7 years ago Closed 7 years ago

Update notification focuses another window

Categories

(Toolkit :: Application Update, defect, P1)

Product:
Toolkit
Component:
Application Update
Version:
55 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: Oriol, Assigned: alexical)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1358363 - Show PanelUI notifications when window focused
59 bytes, text/x-review-board-request
robert.strong.bugs
: review+
Details
screencast.gif
572.99 KB, image/gif
Details 
It has happened multiple times that I'm in one window, the update notification appears and places another window at the top.

To reproduce,
1. Open Firefox (1 window)
2. Open the browser console (Ctrl+Shift+J)
3. Open a new window
4. In the browser console, enter
   gMenuButtonUpdateBadge.showRestartNotification(false)

Result: the old window is focused. So annoying.
Priority: -- → P1
Doug, can you take this bug?
Flags: needinfo?(dothayer)
I can't reproduce on OSX. Depending on your OS this should be either a dupe of bug 1352769 or bug 1339439 (from your description my guess it the former).
Yes, it seems bug 1352769. I don't usually get notifications, except the update ones. So bug 893505 makes this more noticeable.
I think the correct solution regardless of the underlying issue is to only show a badge until the window is focused. This has two benefits:

- Ensure that the doorhanger entry animation is seen by the user, increasing the likelihood that they'll notice and take action.
- If multiple windows are visible to the user, they will only see one doorhanger on the screen, which will ensure that a doorhanger isn't competing for attention with another instance of itself.
Assignee: nobody → dothayer
Flags: needinfo?(dothayer)
Comment on attachment 8860454 [details]
Bug 1358363 - Show PanelUI notifications when window focused

https://reviewboard.mozilla.org/r/132446/#review135464

I pushed this to oak to try it out and there are some test failures
https://treeherder.mozilla.org/#/jobs?repo=oak&tochange=3d0af80058865bfa03581bad2547863d0a434203&fromchange=3d0af80058865bfa03581bad2547863d0a434203

How will this behave on OS X where the problem isn't present?
Attachment #8860454 - Flags: review?(robert.strong.bugs) → review-
Doug, will this approach also fix bug 1355262?
Bram,heads up regarding this change. Please let us know if you are ok with it.
Flags: needinfo?(bram)
From what I understand, the proposed fix sounds great, but I also wasn’t able to reproduce the issue.

Is there an example video I can see, or does it look practically the same as bug 1339439 attachment 8839405 [details]?
Flags: needinfo?(bram)
I don't think there is a video but it brings a browser window to the front when another window (e.g. browser console) is focused on Windows.
Attached image screencast.gif — 
Adding a screen cast.
The notification brings the old window above the newer one and above the browser console. The browser console remains focused, so clicking it does not bring it to the top. You need focus another window first.
(In reply to Oriol from comment #11)
> Created attachment 8860776 [details]

Thanks for the demo video, Oriol. It’s helped me understand the situation.


(In reply to Doug Thayer [:dthayer] from comment #5)
> I think the correct solution regardless of the underlying issue is to only
> show a badge until the window is focused.

This is the correct solution. Because the update doorhanger is not specific to a particular website (unlike a permission doorhanger), we shouldn’t pop it open on all tabs/windows. Just on the one, and more specifically, the one that the user has chosen to focus on.
I've also noticed that neither the doorhanger or the hamburger menu notification is displayed on any other Windows. So, if I have a notification in one window and I close that window then I no longer have anything on the hamburger menu to restart, etc.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #7)
> Doug, will this approach also fix bug 1355262?

It should, yes. Unfortunately I can't reproduce that one, but I don't see how it could happen with this change in place.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #13)
> I've also noticed that neither the doorhanger or the hamburger menu
> notification is displayed on any other Windows. So, if I have a notification
> in one window and I close that window then I no longer have anything on the
> hamburger menu to restart, etc.

How often have you run into this? Was there anything unusual about how you created the extra windows?
I observed same behavior at my end too, as mentioned in comment 13.
(In reply to Doug Thayer [:dthayer] from comment #14)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #7)
> > Doug, will this approach also fix bug 1355262?
> 
> It should, yes. Unfortunately I can't reproduce that one, but I don't see
> how it could happen with this change in place.
> 
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #13)
> > I've also noticed that neither the doorhanger or the hamburger menu
> > notification is displayed on any other Windows. So, if I have a notification
> > in one window and I close that window then I no longer have anything on the
> > hamburger menu to restart, etc.
> 
> How often have you run into this? Was there anything unusual about how you
> created the extra windows?
I tried to reproduce this twice today without success and I'll keep trying.

(In reply to Kanchan Kumari QA from comment #15)
> I observed same behavior at my end too, as mentioned in comment 13.
Can you reproduce and if so with what steps?
Comment on attachment 8860454 [details]
Bug 1358363 - Show PanelUI notifications when window focused

https://reviewboard.mozilla.org/r/132446/#review136348

::: browser/components/customizableui/content/panelUI.js:66
(Diff revision 1)
>      Services.obs.addObserver(this, "fullscreen-nav-toolbox");
>      Services.obs.addObserver(this, "panelUI-notification-main-action");
>      Services.obs.addObserver(this, "panelUI-notification-dismissed");
>  
>      window.addEventListener("fullscreen", this);
> +    window.addEventListener("focus", this);

You should use the activate event for this.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #16)
> (In reply to Kanchan Kumari QA from comment #15)
> > I observed same behavior at my end too, as mentioned in comment 13.
> Can you reproduce and if so with what steps?
Not reproducible for me either today. I am getting menu badge and doorhanger UI in all the Fx open windows (refer to the following video).
Last time when I encountered this issue (as mentioned in comment 13), menu badge or doorhanger UI was getting displayed in one of the open windows and when that same window got closed then there was no menu badge and doorhanger UI in any other opened Fx windows.

I do see original reported issue in this bug i.e. Update notification focuses old window. 

Please see the video here: https://ranisharma22.tinytake.com/sf/MTU0NTEzM181MzQyNjM4
(In reply to Doug Thayer [:dthayer] from comment #14)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #7)
> > Doug, will this approach also fix bug 1355262?
> 
> It should, yes. Unfortunately I can't reproduce that one, but I don't see
> how it could happen with this change in place.
> 
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #13)
> > I've also noticed that neither the doorhanger or the hamburger menu
> > notification is displayed on any other Windows. So, if I have a notification
> > in one window and I close that window then I no longer have anything on the
> > hamburger menu to restart, etc.
> 
> How often have you run into this? Was there anything unusual about how you
> created the extra windows?
I think what I saw was the badge wasn't shown in new windows. Just verified this is the case currently.
Updated the tests and changed to use the "activate" event rather than the "focus" event. I triggered a try, and it's not done yet but so far the issue with the previous patch hasn't manifested.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e40c5fb3dbb61ddb7e85b70b0ec179f627235a1
Comment on attachment 8860454 [details]
Bug 1358363 - Show PanelUI notifications when window focused

https://reviewboard.mozilla.org/r/132446/#review136550

Looks like the try push has failing tests as well as an eslint error
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e40c5fb3dbb61ddb7e85b70b0ec179f627235a1&selectedJob=94247973
Attachment #8860454 - Flags: review?(robert.strong.bugs)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #16)

> Can you reproduce and if so with what steps?

If I open a new window after hamburger menu badge/doorhanger displayed on existing Window then there is no menu badge on new opened window. Is this intended?

Now if I close the window which has hamburger menu badge then there is nothing on hamburger menu on new window to restart. 

I also observed one time, not all opened windows get hamburger menu badge, Please refer to following STR:
1. Launch Nightly which has update.
2. Open about:config and make following changes:
   Change app.update.badgeWaitTime to 10
   Change app.update.lastUpdateTime.background-update-timer to 1
   Change app.update.promptWaitTime to 30
   Change app.update.timerMinimumDelay to 10
3. Exit Firefox.
4. Launch Firefox and quickly open a couple of websites in seperate windows.
5. In approximately a minute or two the hamburger menu badge should be displayed.
6. In approximately 20 more seconds the doorhanger ui should be displayed.

Observation: Out of three opened windows, only two windows get hamburger menu badge and doorhanger ui.
(In reply to Kanchan Kumari QA from comment #23)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #16)
> 
> > Can you reproduce and if so with what steps?
> 
> If I open a new window after hamburger menu badge/doorhanger displayed on
> existing Window then there is no menu badge on new opened window. Is this
> intended?
No and bug 1359733 has been filed for this.
I can't seem to make any real headway on this. It reliably fails on try, and yet I can't get it to fail on my own machine on OSX, Windows, or Linux. I added some logging on activation/deactivation events to diagnose [1], and something is deactivating the main window during the tests. rstrong, do you know of anything that might be causing this, or any gotchas around dealing with focus in tests? I tried to imitate some of the tests for PopupNotifications.jsm, since it also doesn't show doorhangers if the window isn't focused, but it doesn't seem to have the same problems, so I'm wondering if it's something in the update workflow causing it.

1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a053d4ec887d67b7aa93560c27440b87d98080b8
Flags: needinfo?(robert.strong.bugs)
I think I sorted it out. Cleaning up the patch and doing another try run before requesting further review though.
Flags: needinfo?(robert.strong.bugs)
Comment on attachment 8860454 [details]
Bug 1358363 - Show PanelUI notifications when window focused

https://reviewboard.mozilla.org/r/132446/#review141374
Attachment #8860454 - Flags: review?(robert.strong.bugs) → review+
Comment on attachment 8860454 [details]
Bug 1358363 - Show PanelUI notifications when window focused

https://reviewboard.mozilla.org/r/132446/#review141376
Comment on attachment 8860454 [details]
Bug 1358363 - Show PanelUI notifications when window focused

https://reviewboard.mozilla.org/r/132446/#review141378
Pushed by rstrong@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18f88f090a65
Update notification focuses another window. r=rstrong
There were some merge conflicts so I updated the patch

Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/18f88f090a65856b5ce746d6ec5766a7be23f44a
Attachment #8866591 - Flags: review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/21380e8f9c9c for failures in browser/components/customizableui/test/browser_panelUINotifications.js like https://treeherder.mozilla.org/logviewer.html#?job_id=98216522&repo=mozilla-inbound
Comment on attachment 8866591 [details] [diff] [review]
unbitrotted patch as landed on inbound

Doug, if you could please update the patch to latest m-c, request review from me, and I'll land it via autoland after reviewing it. Thanks!
Attachment #8866591 - Attachment is obsolete: true
Flags: needinfo?(dothayer)
Updated.

Quick note: I reverted the change to UP_checkForUpdates because I believe it was unnecessary, and all that was important was the change to showUpdateElevationRequired. The UP_checkForUpdates change was what was causing test_update_wizard.py to fail.
Flags: needinfo?(dothayer)
Makes sense and thanks!
Comment on attachment 8860454 [details]
Bug 1358363 - Show PanelUI notifications when window focused

https://reviewboard.mozilla.org/r/132446/#review141446
Pushed by rstrong@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a941136fcec
Show PanelUI notifications when window focused r=rstrong
https://hg.mozilla.org/mozilla-central/rev/7a941136fcec
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
I tested this bug fix on nightly (Build ID 20170516122050) and found it to be working fine.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: