Closed Bug 1294799 Opened 7 years ago Closed 6 years ago

The bookmark panel can hide when there is a selection made but the mouse leaves the panel

Categories

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

Product:
Firefox
Component:
Bookmarks & History
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- verified
firefox53 --- verified
firefox54 --- verified

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1294799 - Disable the autoclosing of the bookmark popup if there is any interaction with it such as mousedown or keypress.
59 bytes, text/x-review-board-request
mak
: review+
lizzard
: approval-mozilla-beta+
Details
Patch for ESR52
9.35 KB, patch
jcristau
: approval-mozilla-esr52+
Details | Diff | Splinter Review 
STR:
Create a new bookmark
Highlight a selection of the bookmark title
Move the mouse out of the panel

ER:
The panel stays open to allow for changing the bookmark title

AR:
The panel autocloses
Depends on: 1231923
Attachment #8780677 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8780677 [details]
Bug 1294799 - Don't hide the bookmark panel if the user has made a selection change.

https://reviewboard.mozilla.org/r/71324/#review68944

::: browser/base/content/browser-places.js:170
(Diff revision 2)
> +          clearTimeout(this._autoCloseTimer);
> +          this._selectionChanged = true;

Why do we need both the boolean and the clearTimeout? The keydown handler only clears the timeout.
Attachment #8780677 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8780677 [details]
Bug 1294799 - Don't hide the bookmark panel if the user has made a selection change.

https://reviewboard.mozilla.org/r/71324/#review68944

> Why do we need both the boolean and the clearTimeout? The keydown handler only clears the timeout.

That's a good call. We need to do both because after the selection the mouse can move out of the popup which would start the timeout again but we shouldn't autoclose since the user is interacting with the popup still.

We should probably do something similar for the keypress.
Priority: -- → P2
Flags: needinfo?(jaws)
Attachment #8780677 - Attachment is obsolete: true
Flags: needinfo?(jaws)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment on attachment 8835732 [details]
Bug 1294799 - Disable the autoclosing of the bookmark popup if there is any interaction with it such as mousedown or keypress.

https://reviewboard.mozilla.org/r/111350/#review112864

::: browser/base/content/browser-places.js:174
(Diff revision 1)
> +        this._autoCloseTimerEnabled = false;
> +        break;
> +      case "mousedown":
> +        clearTimeout(this._autoCloseTimer);
> +        this._autoCloseTimerEnabled = false;
>          break;

looks like some of these cases could use fall-through, at least compositionstart, input and mousedown.
This code is starting growing a little bit out of the comfort zone, so let's try to avoid repetitions at least.
Attachment #8835732 - Flags: review?(mak77) → review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ab46d343895
Disable the autoclosing of the bookmark popup if there is any interaction with it such as mousedown or keypress. r=mak
https://hg.mozilla.org/mozilla-central/rev/6ab46d343895
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Too late for 51. Mark 51 won't fix.
status-firefox51: affected → wontfix
status-firefox52: --- → affected
Is this worth uplifting to 53?
status-firefox53: --- → affected
Comment on attachment 8835732 [details]
Bug 1294799 - Disable the autoclosing of the bookmark popup if there is any interaction with it such as mousedown or keypress.

Approval Request Comment
[Feature/Bug causing the regression]: improved implementation of bug 1219794
[User impact if declined]: bookmark popup may close unexpectedly while the user is interacting with it
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: make a new bookmark, see that popup closes with no interacting with the popup. remove the bookmark. make a new bookmark, and interact with the popup. confirm that the popup no longer autocloses.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no, been on Nightly for almost 4 weeks now
[Why is the change risky/not risky?]: covered by automated tests, landed on nightly almost 4 weeks ago with no new bugs filed against it.
[String changes made/needed]: none
Attachment #8835732 - Flags: approval-mozilla-aurora?
(In reply to Julien Cristau [:jcristau] from comment #11)
> Is this worth uplifting to 53?

Yes, I have tested this patch on aurora-tip and confirmed that it works as intended. Requested uplift above.
Attachment #8835732 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8835732 [details]
Bug 1294799 - Disable the autoclosing of the bookmark popup if there is any interaction with it such as mousedown or keypress.

That sounds like a super annoying problem for users, let's uplift the fix for beta 2.
Attachment #8835732 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flagging this for manual testing, based on Comment 12. Instructions available in Comment 0 and Comment 12.
Flags: qe-verify+
I couldn't reproduce this issue on Windows 10 64-bit.
This is reproducible under Mac OS X 10.11 and I confirm the fix on Firefox 53 beta 2 (downloaded from taskcluster) and latest Aurora 54.0a2 2017-03-13.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
status-firefox54: fixed → verified
Flags: qe-verify+
I'm thinking we want this on esr52 too.
Flags: needinfo?(jaws)
Attached patch Patch for ESR52 — — Splinter Review 
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: annoying user-visible bug with the bookmarks menu
User impact if declined: bookmark edit menu can close unexpectedly
Fix Landed on Version: 54, and uplifted to 53
Risk to taking this patch (and alternatives if risky): none expected, stable on m-c, and aurora/beta builds. includes automated test.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(jaws)
Attachment #8853133 - Flags: approval-mozilla-esr52?
Comment on attachment 8853133 [details] [diff] [review]
Patch for ESR52

bookmark menu fix for esr52
Attachment #8853133 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Petruta, could you please verify this on 52.1esr as well?
Flags: needinfo?(petruta.rasa)
Verified as fixed using Firefox 52.1esr under Mac OS X 10.11.
status-firefox-esr52: fixed → verified
Flags: needinfo?(petruta.rasa)
You need to log in before you can comment on or make changes to this bug.