Closed Bug 1327938 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 + verified
firefox53 + verified

People

(Reporter: arni2033, Assigned: jaws)

References

Details

(Keywords: regression, ux-consistency)

Attachments

(1 file)

>>>   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).
No longer blocks: 1277113
Component: Untriaged → Bookmarks & History
[Tracking Requested - why for this release]: Bad user experience due to bug 1219794.
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
Assignee: nobody → jaws
Status: NEW → ASSIGNED
(In reply to atlanto from comment #2)

Thank you for the detailed debugging information!
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.
Attachment #8827983 - Flags: review?(mak77) → review+
Pushed by jwein@mozilla.com:
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 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
Attachment #8827983 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c6673f7e07d3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Tracking 52/53+ to improve the user experience.
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+
Attachment #8827983 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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]
You need to log in before you can comment on or make changes to this bug.