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

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Bookmarks & History
P3
normal
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: jimm, Assigned: matrixisreal, Mentored, NeedInfo)

Tracking

Trunk
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox46 affected, firefox53 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
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

10 months ago
Yay, forget button bugs. Verdi, what do you think?
Component: General → Bookmarks & History
Flags: needinfo?(mverdi)

Comment 2

10 months ago
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.
Flags: needinfo?(mverdi)

Comment 3

10 months ago
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.
Mentor: gijskruitbosch+bugs@gmail.com
Flags: needinfo?(mverdi)
Priority: -- → P3

Updated

10 months ago
Whiteboard: [lang=js]
(Reporter)

Comment 4

10 months ago
(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

10 months ago
(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)
Mentor: gijskruitbosch+bugs@gmail.com

Updated

10 months ago
Whiteboard: [lang=js]

Comment 6

10 months ago
(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.
Flags: needinfo?(mverdi)

Updated

10 months ago
Mentor: gijskruitbosch+bugs@gmail.com
Whiteboard: [lang=js]

Updated

10 months ago
Summary: Consider removing the "Thanks" popup after forgetting history → Automatically dismiss the "Thanks" popup after forgetting history if it isn't interacted with.

Comment 7

6 months ago
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.
(Assignee)

Comment 8

6 months ago
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

6 months ago
(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.
(Assignee)

Comment 10

6 months ago
Hi,
Could you tell me how to debug this , because the browser tollbox is closed whenever i click forget
(Assignee)

Updated

6 months ago
Assignee: nobody → pavankarthikboddeda

Comment 11

6 months ago
(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).
(Assignee)

Comment 12

6 months ago
Created attachment 8821651 [details] [diff] [review]
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.
Attachment #8821651 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 13

6 months ago
(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

6 months ago
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.
Attachment #8821651 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 15

6 months ago
Created attachment 8822777 [details] [diff] [review]
v1.1.patch

Change: keydown event closes the popup which i guess is more appropriate than clearing the time out.
Attachment #8821651 - Attachment is obsolete: true
Attachment #8822777 - Flags: ui-review?(mverdi)
Attachment #8822777 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 16

6 months ago
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

6 months ago
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. :-)
Attachment #8822777 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 18

6 months ago
Created attachment 8822907 [details] [diff] [review]
v1.2.patch

Changes: removed keydown Listener after the popup is closed,
         removed timer when keydown
Attachment #8822777 - Attachment is obsolete: true
Attachment #8822777 - Flags: ui-review?(mverdi)
Attachment #8822907 - Flags: ui-review?(mverdi)
Attachment #8822907 - Flags: review?(gijskruitbosch+bugs)

Comment 19

6 months ago
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. :-)
Attachment #8822907 - Flags: review?(gijskruitbosch+bugs) → feedback+

Comment 20

6 months ago
Eh, forgot the trypush link:

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

Updated

6 months ago
Attachment #8822907 - Flags: ui-review?(mverdi) → ui-review+
(Assignee)

Comment 21

6 months ago
Created attachment 8823595 [details] [diff] [review]
v1.3.patch

Pretty much same as what you have mentioned.
Attachment #8822907 - Attachment is obsolete: true
Attachment #8823595 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

6 months ago
Status: NEW → ASSIGNED

Comment 22

6 months ago
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. :-)
Attachment #8823595 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 23

6 months ago
Created attachment 8823648 [details] [diff] [review]
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
Attachment #8823595 - Attachment is obsolete: true
Attachment #8823648 - Flags: review?(gijskruitbosch+bugs)

Comment 24

6 months ago
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
Attachment #8823648 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 25

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

Comment 26

6 months ago
(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 .
Keywords: checkin-needed

Comment 27

6 months ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 28

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/43429fbedc41
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
(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?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 30

6 months ago
(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
Flags: needinfo?(gijskruitbosch+bugs)
(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

6 months ago
(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?
Flags: needinfo?(mverdi)
You need to log in before you can comment on or make changes to this bug.