1235547 - Automatically dismiss the "Thanks" popup after forgetting history if it isn't interacted with.

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Jim Mathies [:jimm]]

The "Thanks" popup that displays after using the "forget the last" history feature serves no purpose other than adding an annoying step between restart and getting back to browsing. I'd like to suggest we get rid of it.

Comment 1

[:Gijs (he/him)]

Yay, forget button bugs. Verdi, what do you think?

Comment 2

[Verdi [:verdi] Best to slack me]

Well the doorhanger does let you know the previous action was successful. We could add a time out to it like we use for the bookmark panel and the pocket panel.

Comment 3

[:Gijs (he/him)]

OK, I can mentor someone to create a timeout for this panel. Verdi, how long? 2 seconds after the popup has been fully shown? 5 seconds? Something else?

To fix this bug, we need to add a "popupshown" event listener to the popup element in this code:

http://searchfox.org/mozilla-central/rev/064025c802c22cd5ad122746733cbd34ea47393c/browser/base/content/browser.js#8002-8012

In the "popupshown" event listener, we'll need to start off a timeout of the duration Verdi suggested, and when the timeout fires, we should close the popup (call PanicButtonNotifier.close() ). We should save the return value of setTimeout on the PanicButtonNotifier object, and use it to cancel the timeout once the user mouses over the panel, by attaching a mousemove handler to the popup as well that takes care of that.

Comment 4

[Jim Mathies [:jimm]]

(In reply to Verdi [:verdi] from comment #2)
> Well the doorhanger does let you know the previous action was successful. We
> could add a time out to it like we use for the bookmark panel and the pocket
> panel.

Rather than message the user that the operation was a success (which I'm guessing is 99.999999% of the time), we could message if an error occurs instead. Save the user from an annoying prompt in the process?

Comment 5

[:Gijs (he/him)]

(In reply to Jim Mathies [:jimm] from comment #4)
> (In reply to Verdi [:verdi] from comment #2)
> > Well the doorhanger does let you know the previous action was successful. We
> > could add a time out to it like we use for the bookmark panel and the pocket
> > panel.
> 
> Rather than message the user that the operation was a success (which I'm
> guessing is 99.999999% of the time), we could message if an error occurs
> instead. Save the user from an annoying prompt in the process?

When would this prompt be that annoying? If used repeatedly? Because in that case, it feels like it'd be more productive to nudge the user towards private browsing mode instead...

(un-mentoring to avoid people working on this when it's not clear what we want - let's wait for verdi to weigh in)

Comment 6

[Verdi [:verdi] Best to slack me]

(In reply to Jim Mathies [:jimm] from comment #4)
> Rather than message the user that the operation was a success (which I'm
> guessing is 99.999999% of the time), we could message if an error occurs
> instead. Save the user from an annoying prompt in the process?

I think it's better, especially since this is sensitive data, for the browser to confirm that the thing you wanted to do was in fact successfully done. 

(In reply to :Gijs Kruitbosch from comment #3)
> OK, I can mentor someone to create a timeout for this panel. Verdi, how
> long? 2 seconds after the popup has been fully shown? 5 seconds? Something
> else?

Let's use 3 seconds which is what we do for the bookmarks and the pocket panel. Clicking "thanks" will still dismiss the panel immediately.

Comment 7

[Dão Gottwald [:dao]]

FWIW, I think 3 seconds is likely too short for users with dyslexia or who may be distracted and slow to read the message. Also not sure how this works for screen reader users. Back in the day we used to avoid automatically dismissing messages altogether for this reason.

Since we're already doing this in the bookmarks panel, I suppose it's not a blocker for this bug :/ , but I'd recommend a follow-up being filed and a11y peers being consulted.

Comment 8

[Pavan Karthik [:matrixisreal]]

Hi Gijs,
I have started working on this bug, and could use some help.
I have few doubts, will u please clear them.

1.Will Adding setTimeout(this.close(), 3000) without any event listener in notify() close the popup in 3sec? I was hoping so, but it didnt work.what might be the prob?

2.Then I tried this
diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
     // Display notification panel here...
     try {
       let popup = document.getElementById("panic-button-success-notification");
       popup.hidden = false;
+      popup.addEventListener("popupshown",func(){
+        var timer = setTimeout(this.close(), 3000);
+      });
       let widget = CustomizableUI.getWidget("panic-button").forWindow(window);
       let anchor = widget.anchor;

But this didnt work too, infact the whole browser seemed to get corrupted and nothing worked,
Can you tell me what i am Doing wrong?

Comment 9

[:Gijs (he/him)]

(In reply to Pavan Karthik from comment #8)
> Hi Gijs,
> I have started working on this bug, and could use some help.
> I have few doubts, will u please clear them.
> 
> 1.Will Adding setTimeout(this.close(), 3000) without any event listener in
> notify() close the popup in 3sec? I was hoping so, but it didnt work.what
> might be the prob?

You will want to call popup.hidePopup() instead of this.close().

> 2.Then I tried this
> diff --git a/browser/base/content/browser.js
> b/browser/base/content/browser.js
> --- a/browser/base/content/browser.js
> +++ b/browser/base/content/browser.js
>      // Display notification panel here...
>      try {
>        let popup =
> document.getElementById("panic-button-success-notification");
>        popup.hidden = false;
> +      popup.addEventListener("popupshown",func(){
> +        var timer = setTimeout(this.close(), 3000);
> +      });
>        let widget =
> CustomizableUI.getWidget("panic-button").forWindow(window);
>        let anchor = widget.anchor;
> 
> But this didnt work too, infact the whole browser seemed to get corrupted
> and nothing worked,
> Can you tell me what i am Doing wrong?

Well, three things:

One, you use "func()" which isn't valid JS. Did you mean "function()"?

Two, this.close() will probably call window.close(), so as I noted above, I think you want popup.hidePopup.

Third, the way you wrote this the closing function (this.close() in your example) is called immediately. You should pass a function to setTimeout, so perhaps something like this:

setTimeout(() => popup.hidePopup(), 3000);

would work.

Comment 10

[Pavan Karthik [:matrixisreal]]

Hi,
Could you tell me how to debug this , because the browser tollbox is closed whenever i click forget

Comment 11

[:Gijs (he/him)]

(In reply to Pavan Karthik from comment #10)
> Hi,
> Could you tell me how to debug this , because the browser tollbox is closed
> whenever i click forget

Ah, hm, yeah... I think the best suggestion I can make is to use print-style debugging. If you use something like:

Cu.reportError("some string");

or

Cu.reportError("stuff: " + someVariable);

you can print strings to the browser console (ctrl-shift-j to open, or use the web developer menu).

Comment 12

[Pavan Karthik [:matrixisreal]]

Attachment: v1.patch

Hope this is what u wanted
As Dao mentioned, 3 Sec looks a bit short window, if user is not so aware of the popup.

Comment 13

[Pavan Karthik [:matrixisreal]]

(In reply to :Gijs (gone 24 dec - 3 jan) from comment #11)
> (In reply to Pavan Karthik from comment #10)
> > Hi,
> > Could you tell me how to debug this , because the browser tollbox is closed
> > whenever i click forget
> 
> Ah, hm, yeah... I think the best suggestion I can make is to use print-style
> debugging. If you use something like:
> 
> Cu.reportError("some string");
> 
> or
> 
> Cu.reportError("stuff: " + someVariable);
> 
> you can print strings to the browser console (ctrl-shift-j to open, or use
> the web developer menu).

yeah, Im doing almost the same(using console.log())
But i was hoping if there was something like remote debugging.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8821651 [details] [diff] [review]
v1.patch

Review of attachment 8821651 [details] [diff] [review]:
-----------------------------------------------------------------

For the next patch, we should get a UI-review given the comments so far. Perhaps a longer timeout is warranted. Can you push to try? If so, please do so on the next iteration and request ui-review from Verdi.

::: browser/base/content/browser.js
@@ +8050,5 @@
>      // Display notification panel here...
>      try {
>        let popup = document.getElementById("panic-button-success-notification");
>        popup.hidden = false;
> +      popup.addEventListener("popupshown",function(){

Nits: please run eslint (./mach eslint browser/base would do, I think). You'll want:

a space before 'function'
a space after the '()'
no trailing spaces at the end of the next line.

@@ +8055,5 @@
> +        PanicButtonNotifier.timer = setTimeout(() => popup.hidePopup(), 3000);       
> +      });
> +      popup.addEventListener("mouseover", function(){
> +        clearTimeout(PanicButtonNotifier.timer);
> +      });

We should probably also listen for 'keydown' inside the popup.

Comment 15

[Pavan Karthik [:matrixisreal]]

Attachment: v1.1.patch

Change: keydown event closes the popup which i guess is more appropriate than clearing the time out.

Comment 16

[Pavan Karthik [:matrixisreal]]

Hi Gijs,
Sorry for the late turn around,I was busy with college works.
I havent pushed it to try server(i dont have permissions yet.).Ill apply form them soon,So for timebeing could you do that for me?
I hope the patch is ok.
Thanks :D

Comment 17

[:Gijs (he/him)]

Comment on attachment 8822777 [details] [diff] [review]
v1.1.patch

Review of attachment 8822777 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Pavan Karthik from comment #16)
> Hi Gijs,
> Sorry for the late turn around,I was busy with college works.
> I havent pushed it to try server(i dont have permissions yet.).Ill apply
> form them soon,So for timebeing could you do that for me?
> I hope the patch is ok.
> Thanks :D

Thanks for the patch. I actually think that keydown should clear the timeout - much like mouseover, if the user is trying to interact with the popup via the keyboard, we should probably not close it.

Note that you will need to remove the keydown event listener when the popup closes, or it'll stay there forever while the browser window is up.

No worries about how long it takes, I was away on holiday anyway. :-)

Comment 18

[Pavan Karthik [:matrixisreal]]

Attachment: v1.2.patch

Changes: removed keydown Listener after the popup is closed,
         removed timer when keydown

Comment 19

[:Gijs (he/him)]

Comment on attachment 8822907 [details] [diff] [review]
v1.2.patch

Review of attachment 8822907 [details] [diff] [review]:
-----------------------------------------------------------------

This is good enough for verdi to ui-review, I think. I've pushed this to try: 


However, there are some issues I've outlined below meaning we'll need to iterate on it some more...

::: browser/base/content/browser.js
@@ +8058,5 @@
> +        clearTimeout(PanicButtonNotifier.timer);
> +      });
> +      window.addEventListener("keydown", function keydownHandler() {
> +        clearTimeout(PanicButtonNotifier.timer);
> +        if (popup.hidden) {

Popups are a really confusing part of our platform. Unfortunately element.hidden doesn't correspond to whether the popup is being shown or not, but with whether or not its hidden attribute is set. In the markup, the hidden attribute is present to avoid it and its layout impacting startup time, and so when we try to show the popup we set it to hidden = false first. Then below, where we call `openPopup` is where the popup is actually shown.

All this to say that this isn't the right way to detect whether the popup is closing. Here's what I think we should do:

1) have a single named function that we attach for both mouseover and keydown, e.g.:

let onUserInteractsWithPopup = e => {
  clearTimeout(PanicButtonNotifier.timer);
  removeListeners();
};

and attach it to the mouseover and keydown events:

popup.addEventListener("mouseover", onUserInteractsWithPopup);
popup.addEventListener("keydown", onUserInteractsWithPopup);

2) have another function to remove the listeners called 'removeListeners':

let removeListeners = () => {
  popup.removeEventListener("mouseover", onUserInteractsWithPopup);
  popup.removeEventListener("keydown", onUserInteractsWithPopup);
  popup.removeEventListener("popuphidden", removeListeners);
};

and attach that function to the 'popuphidden' event:

popup.addEventListener("popuphidden", removeListeners);



In fact, thinking about it, it would be neatest to also name and remove the `popupshown` event listener from the `removeListeners` method.

Finally, we should also call `removeListeners()` in the function that runs after the 3000ms timeout. :-)

Comment 20

[:Gijs (he/him)]

Eh, forgot the trypush link:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0e82c28abd7a840271e94115b27009ca5ae4852

Comment 21

[Pavan Karthik [:matrixisreal]]

Attachment: v1.3.patch

Pretty much same as what you have mentioned.

Comment 22

[:Gijs (he/him)]

Comment on attachment 8823595 [details] [diff] [review]
v1.3.patch

Review of attachment 8823595 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +8062,5 @@
> +      // To prevent the popup from closing when user tries to interact with the
> +      // popup using mouse or keyboard.
> +      popup.addEventListener("mouseover", onUserInteractsWithPopup);
> +      window.addEventListener("keydown", onUserInteractsWithPopup);
> +      popup.addEventListener("popuphidden", removeListeners);

Please add these listeners after you define them. I don't think this order actually works. Did you test this patch?

@@ +8063,5 @@
> +      // popup using mouse or keyboard.
> +      popup.addEventListener("mouseover", onUserInteractsWithPopup);
> +      window.addEventListener("keydown", onUserInteractsWithPopup);
> +      popup.addEventListener("popuphidden", removeListeners);
> +      let onUserInteractsWithPopup = e => {

Nit: you don't use 'e' here, so just use () instead. :-)

Comment 23

[Pavan Karthik [:matrixisreal]]

Attachment: v1.4.patch

Hi,
Yes, I have tested the previous patch and everything worked fine.
But anyways i have fixed the nits you have mentioned.

Pavan

Comment 24

[:Gijs (he/him)]

Comment on attachment 8823648 [details] [diff] [review]
v1.4.patch

Review of attachment 8823648 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good - trypush:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=66d80f3f5f36596ce32951f6157695bbb633ed36

Comment 25

[Pavan Karthik [:matrixisreal]]

Hi Gijs,
Could you suggest something a bit more challenging for me to work on?
Thanks :D

Comment 26

[:Gijs (he/him)]

(In reply to Pavan Karthik [:matrixisreal] from comment #25)
> Hi Gijs,
> Could you suggest something a bit more challenging for me to work on?
> Thanks :D

How about https://bugzilla.mozilla.org/show_bug.cgi?id=1258797#c4 ? Or, if you know CSS and have access to Windows 7 or 8, https://bugzilla.mozilla.org/show_bug.cgi?id=1111138 ? If you want something much bigger, you could try https://bugzilla.mozilla.org/show_bug.cgi?id=1224137 .

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43429fbedc41
Automatically dismiss the Thanks popup after forgetting history if it isn't interacted with. r=Gijs

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/43429fbedc41

Comment 29

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #14)
> Comment on attachment 8821651 [details] [diff] [review]
> v1.patch
> 
> Review of attachment 8821651 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For the next patch, we should get a UI-review given the comments so far.
> Perhaps a longer timeout is warranted. Can you push to try? If so, please do
> so on the next iteration and request ui-review from Verdi.

Looks like this never happened?

Comment 30

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #29)
> (In reply to :Gijs from comment #14)
> > Comment on attachment 8821651 [details] [diff] [review]
> > v1.patch
> > 
> > Review of attachment 8821651 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > For the next patch, we should get a UI-review given the comments so far.
> > Perhaps a longer timeout is warranted. Can you push to try? If so, please do
> > so on the next iteration and request ui-review from Verdi.
> 
> Looks like this never happened?

It did? https://bugzilla.mozilla.org/show_bug.cgi?id=1235547#a32090866_368084

Comment 31

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #30)
> (In reply to Dão Gottwald [:dao] from comment #29)
> > (In reply to :Gijs from comment #14)
> > > Comment on attachment 8821651 [details] [diff] [review]
> > > v1.patch
> > > 
> > > Review of attachment 8821651 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > For the next patch, we should get a UI-review given the comments so far.
> > > Perhaps a longer timeout is warranted. Can you push to try? If so, please do
> > > so on the next iteration and request ui-review from Verdi.
> > 
> > Looks like this never happened?
> 
> It did? https://bugzilla.mozilla.org/show_bug.cgi?id=1235547#a32090866_368084

That link doesn't seem to lead anywhere, but now that I look harder I see a ui-review+ with no word on why 3 seconds are okay considering accessibility...

Comment 32

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #31)
> (In reply to :Gijs from comment #30)
> > (In reply to Dão Gottwald [:dao] from comment #29)
> > > (In reply to :Gijs from comment #14)
> > > > Comment on attachment 8821651 [details] [diff] [review]
> > > > v1.patch
> > > > 
> > > > Review of attachment 8821651 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > For the next patch, we should get a UI-review given the comments so far.
> > > > Perhaps a longer timeout is warranted. Can you push to try? If so, please do
> > > > so on the next iteration and request ui-review from Verdi.
> > > 
> > > Looks like this never happened?
> > 
> > It did? https://bugzilla.mozilla.org/show_bug.cgi?id=1235547#a32090866_368084
> 
> That link doesn't seem to lead anywhere,

Ugh, filed bug 1329189.

> but now that I look harder I see a
> ui-review+ with no word on why 3 seconds are okay considering
> accessibility...

-> Verdi?

Comment 33

[Verdi [:verdi] Best to slack me]

The justification for 3 seconds is only that we use it in a number of other places (e.g. pocket, bookmarks). Longer time outs or acknowledgement buttons might be things to consider. If so we should do that wholisticly and in another bug.