1327938 - "New bookmark" popup disappears if I reopen it after creating bookmark - round 2

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 50, 32bit, ID 20160714030208 (2016-07-14)
STR_1:
1. Open http://example.org/ in a new tab
2. Move mouse pointer exactly where "Done" button is located when "New bookmark" popup is open
3. Press Ctrl+D
4. Press Enter or click "Done" (you can even move mouse within "Done" button, but not outside)
5. Press Ctrl+D
 [you must perform Steps 3-5 in less than 4 seconds]
6.(bonus) Click in the field "Name" 

AR:  After Step 5 (or after Step 6, if you perform it) "new bookmark" popup disappears
ER:  Widget should react according to the last interaction, i.e. bookmarks panel shouldn't hide
    [this cool expectation can be applied to the cases in Note (1) and probably more]

Use case:
 I decided to bookmark a page, then I realized that I want to edit bookmark (Step 5)

This bug is regression from bug 1219794. Regression range:
> https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9e33d8c48b5ca93ca1937eba4220f681a0f05ec&tochange=7a2db11cc81934507f730ae24429a41be6202438

Notes:
1) Many widgets in the Mozilla's browser works like described above. If user keeps constantly moving
   mouse pointer above fullscreen notification - it still hides. If user hovers "Push Notification"
   in the beginning/end - the notification is hardly visible (bug 1184790).

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: Bad user experience due to bug 1219794.

Comment 2

[atlanto]

I can reproduce this on Windows10 but not on Linux.

Here is another STR for Windows,
1. Click the star icon
2. Click "Remove Bookmark" and move mouse pointer a few pixel or 1mm or ... little bit anyway
3. Hit Ctrl+D
4. Start typing

or
1. Click the star icon and move mouse pointer quickly to the position where buttons will appear
2. Start editing (type something, or move mouse pointer to Folder and click, or such)
(this is hard to reproduce the problem but it is almost real steps. "to the position where buttons will appear" is not intentional of course but it will happen occasionally)

Well. *ell.

Inspection Code:
diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -57,19 +57,21 @@ var StarUI = {
     });
   },
 
   // nsIDOMEventListener
   handleEvent(aEvent) {
     switch (aEvent.type) {
       case "mousemove":
         clearTimeout(this._autoCloseTimer);
+        console.log(aEvent.type+": timer "+this._autoCloseTimer+" cleared");
         break;
       case "popuphidden":
         clearTimeout(this._autoCloseTimer);
+        console.log(aEvent.type+": timer "+this._autoCloseTimer+" cleared");
         if (aEvent.originalTarget == this.panel) {
           if (!this._element("editBookmarkPanelContent").hidden)
             this.quitEditMode();
 
           if (this._anchorToolbarButton) {
             this._anchorToolbarButton.removeAttribute("open");
             this._anchorToolbarButton = null;
           }
@@ -100,16 +102,17 @@ var StarUI = {
 
             PlacesTransactions.RemoveBookmarksForUrls([this._uriForRemoval])
                               .transact().catch(Cu.reportError);
           }
         }
         break;
       case "keypress":
         clearTimeout(this._autoCloseTimer);
+        console.log(aEvent.type+": timer "+this._autoCloseTimer+" cleared");
 
         if (aEvent.defaultPrevented) {
           // The event has already been consumed inside of the panel.
           break;
         }
 
         switch (aEvent.keyCode) {
           case KeyEvent.DOM_VK_ESCAPE:
@@ -145,19 +148,24 @@ var StarUI = {
         // auto-close if new and not interacted with
         if (this._isNewBookmark) {
           // 3500ms matches the timeout that Pocket uses in
           // browser/extensions/pocket/content/panels/js/saved.js
           let delay = 3500;
           if (this._closePanelQuickForTesting) {
             delay /= 10;
           }
-          this._autoCloseTimer = setTimeout(() => {
-            this.panel.hidePopup();
-          }, delay);
+          this._autoCloseTimer = ((panel) => {
+            var timerId = setTimeout(() => {
+              console.log("timer "+timerId+" timed out("+delay+")");
+              panel.hidePopup();
+            }, delay);
+            return timerId;
+          })(this.panel);
+          console.log(aEvent.type+"("+aEvent.target.id+","+aEvent.explicitOriginalTarget.id+"): timer "+this._autoCloseTimer+" started");
         }
         break;
     }
   },
 
   _overlayLoaded: false,
   _overlayLoading: false,
   showEditBookmarkPopup: Task.async(function* (aNode, aAnchorElement, aPosition, aIsNewBookmark) {

Inspection Steps:
(described above, my STR)

Console Log: (a sample)
popupshown(editBookmarkPanel,editBookmarkPanel): timer 21 started
mousemove: timer 21 cleared
mouseout(editBookmarkPanel,editBookmarkPanel): timer 22 started
mousemove: timer 22 cleared
mouseout(editBookmarkPanel,editBookmarkPanelHeader): timer 23 started
mousemove: timer 23 cleared
mouseout(editBookmarkPanel,editBookmarkPanelRemoveButton): timer 24 started
mousemove: timer 24 cleared
popuphidden: timer 24 cleared
popupshown(editBookmarkPanel,editBookmarkPanel): timer 25 started
mouseout(editBookmarkPanel,editBookmarkPanelRemoveButton): timer 26 started
mousemove: timer 26 cleared
mouseout(editBookmarkPanel,): timer 27 started
mousemove: timer 27 cleared
keypress: timer 27 cleared
timer 25 timed out(3500)
popuphidden: timer 27 cleared

The Point To Be Considered Here:
>popupshown(editBookmarkPanel,editBookmarkPanel): timer 25 started
>mouseout(editBookmarkPanel,editBookmarkPanelRemoveButton): timer 26 started
>mousemove: timer 26 cleared
...
>timer 25 timed out(3500)
Also
>keypress: timer 27 cleared
>timer 25 timed out(3500)

(to mention other considerable points is not my business)
fwiw, inspected on
>$ hg parent
>changeset:   329666:6a23526fe516

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1327938 - Clear the existing timeout before creating a new one, as we should only have one autoclose timer running at a time.

Review commit: https://reviewboard.mozilla.org/r/105536/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105536/

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to atlanto from comment #2)

Thank you for the detailed debugging information!

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8827983 [details]
Bug 1327938 - Clear the existing timeout before creating a new one, as we should only have one autoclose timer running at a time.

https://reviewboard.mozilla.org/r/105536/#review106374

make A LOT of sense.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6673f7e07d3
Clear the existing timeout before creating a new one, as we should only have one autoclose timer running at a time. r=mak

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8827983 [details]
Bug 1327938 - Clear the existing timeout before creating a new one, as we should only have one autoclose timer running at a time.

Approval Request Comment
[Feature/Bug causing the regression]: regression from bookmark pop-up autoclose g which landed in 47
[User impact if declined]: pop-up can close mysteriously
[Is this code covered by automated tests?]: yes, but not this specific change as it's too difficult to test this case through automation
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: No, this is a simple call to remove old timer before creating a new one 
[Why is the change risky/not risky?]: See above
[String changes made/needed]: none

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c6673f7e07d3

Comment 9

[Marcia Knous [:marcia]]

Tracking 52/53+ to improve the user experience.

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8827983 [details]
Bug 1327938 - Clear the existing timeout before creating a new one, as we should only have one autoclose timer running at a time.

only keep one timer to close "new bookmark" popup, aurora52+

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5ec6cfbdd4e1

Comment 12

[Kazi Nuzhat Tasnem]

I have reproduced this bug with Firefox nightly 50.0a1(build id:20160714030208)on
windows 7(64 bit)

Verified this bug as fixed with Firefox aurora 52.0a2(build id:20170123004004)
user agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0

Verified this bug as fixed with Firefox nightly 53.0a1(build id:20170123030211)
user agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0

[bugday-20170201]

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Thanks!