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

Status: VERIFIED FIXED

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

Description

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

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

Comment 1

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

Attachment: Bug 1294799 - Don't hide the bookmark panel if the user has made a selection change.

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

Comment 2

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

Comment on attachment 8780677 [details]
Bug 1294799 - Don't hide the bookmark panel if the user has made a selection change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71324/diff/1-2/

Comment 3

[:Gijs (he/him)]

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.

Comment 4

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

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.

Comment 5

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

Attachment: Bug 1294799 - Disable the autoclosing of the bookmark popup if there is any interaction with it such as mousedown or keypress.

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

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

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

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111350/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/6ab46d343895

Comment 10

[Gerry Chang [:gchang]]

Too late for 51. Mark 51 won't fix.

Comment 11

[Julien Cristau [:jcristau]]

Is this worth uplifting to 53?

Comment 12

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

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

Comment 13

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

(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.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/69e5687ef053

Comment 16

[Andrei Vaida [:avaida]]

Flagging this for manual testing, based on Comment 12. Instructions available in Comment 0 and Comment 12.

Comment 17

[Petruta Horea [:phorea], Desktop QA]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

I'm thinking we want this on esr52 too.

Comment 19

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

Attachment: Patch for ESR52

[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.

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 8853133 [details] [diff] [review]
Patch for ESR52

bookmark menu fix for esr52

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/0617b074ec3d

Comment 22

[Andrei Vaida [:avaida]]

Petruta, could you please verify this on 52.1esr as well?

Comment 23

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed using Firefox 52.1esr under Mac OS X 10.11.