1335043 - The bookmark panel closes automatically if the mouse is located over the panel when it opens

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

From https://bugzilla.mozilla.org/show_bug.cgi?id=1290011#c41

> I noticed also that it prematurely times out if I quickly move the mouse to 
> the dialog box immediately after clicking the bookmark option from the main 
> menu.

> If I leave the mouse perfectly still upon clicking "bookmark this page" and 
> then wait until after the dialogue box has fully displayed before moving my 
> mouse over to it to begin interacting with it, the dialogue box will remain 
> until I am finished with it.

> Therefore, I no longer believe it is an intermittent problem and this is the 
> process to replicate the problem: moving the mouse to the dialogue box 
> before/while it opens fails to trigger the mouse-over detection so that it'll 
> cancel the 3-second timeout.

Comment 1

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

Attachment: Bug 1335043 - Prevent the bookmark panel from closing automatically if the mouse is hovering over it.

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

Comment 2

[Marco Bonardo [:mak]]

I'm not totally convinced this is a bug. Sure, the mouse is on the panel, but still there is no activity for 3 seconds, no mousemove, no keyboard interaction. The user is _not_ using the panel for that whole time. This is exactly why we wanted to autoclose it.

I could see this bug the opposite way: after fixing this bug, I click and inadvertently move the mouse down where the popup will appear. The panel doesn't close anymore even if I don't need it and I'm not interacting with it. That'd look like a regression to me: why it doesn't close if I'm not interacting with it?

Note we don't use mouseover, we use mousemove, that means "while the panel is open, the user is moving his mouse". There is no mouseover involvement here, nor we cared (so far) about what the user does when the panel is not visible.

Imo, this is a wontfix. I mean, both cases can be seen as correct, it's a matter of subjective preference. And in my vision they have the same identical value. Thus, in lack of an explicit UX requirement I'd be more prone to refuse adding code complication where the added value is so low.

Comment 3

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

Hovering the mouse over a panel is a common behavior to delay closing of an notification. We implemented something similar for the WebNotification API (see bug 888380).

Comment 4

[Marco Bonardo [:mak]]

The web notification case is different, due to its positioning. My main concern was that the button is on the top-right of the window, the panel opens on the way back from the star button to content.

Though, by having a second look to the code, we also handle mouseout, that pretty much solves my concern, since it'll try to close again.
So this should do, will review shortly.

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8831705 [details]
Bug 1335043 - Prevent the bookmark panel from closing automatically if the mouse is hovering over it.

https://reviewboard.mozilla.org/r/108256/#review109892

::: browser/base/content/browser-places.js:181
(Diff revision 1)
>            clearTimeout(this._autoCloseTimer);
>            this._autoCloseTimer = setTimeout(() => {
> +            if (this.panel.mozMatchesSelector(":hover")) {
> +              return;
> +            }
>              this.panel.hidePopup();

nit: would be shorter as:
  if (!this.panel.mozMatchesSelector(":hover")) {
    this.panel.hidePopup();
  }

::: browser/base/content/test/general/browser_bookmark_popup.js:375
(Diff revision 1)
> +add_task(function* mouse_hovering_panel_should_prevent_autoclose() {
> +  if (AppConstants.platform != "win") {
> +    // This test requires synthesizing native mouse movement which is
> +    // best supported on Windows.
> +    return;
> +  }

add_task now supports an optional object with option to skip tests:
add_task({
  skip_if: () => AppConstants.platform != "win",
}, function* (...

Comment 6

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

Comment on attachment 8831705 [details]
Bug 1335043 - Prevent the bookmark panel from closing automatically if the mouse is hovering over it.

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

Comment 7

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

Comment on attachment 8831705 [details]
Bug 1335043 - Prevent the bookmark panel from closing automatically if the mouse is hovering over it.

https://reviewboard.mozilla.org/r/108256/#review109892

> add_task now supports an optional object with option to skip tests:
> add_task({
>   skip_if: () => AppConstants.platform != "win",
> }, function* (...

Confirmed over IRC that this is only available for xpcshell tests.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc22dc0fe963
Prevent the bookmark panel from closing automatically if the mouse is hovering over it. r=mak

Comment 9

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

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

Comment 10

[Fahima Zulfath]

[bugday-20170301]
OPERATING SYSTEM: Windows 10
BROWSER: Nightly 54
  The bug is verified!!!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Should we consider this for 53 backport too?

Comment 12

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

Comment on attachment 8831705 [details]
Bug 1335043 - Prevent the bookmark panel from closing automatically if the mouse is hovering over it.

I've confirmed that the patch landed here will rebase fine to beta and the tests still pass locally on the beta tree for me.

Approval Request Comment
[Feature/Bug causing the regression]: followup from bug 1219794
[User impact if declined]: the bookmark popup closes randomly while the user is using
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[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, been on nightly and aurora for a long time now
[Why is the change risky/not risky?]: no, been on nightly and aurora and covered by multiple automated tests
[String changes made/needed]: none

Comment 13

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 8831705 [details]
Bug 1335043 - Prevent the bookmark panel from closing automatically if the mouse is hovering over it.

Regression fix, let's bring it to beta 2.

Comment 14

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

https://hg.mozilla.org/releases/mozilla-beta/rev/fe89c5aec076

Comment 15

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

I messed up some minor rebase issues when I landed this. Backed out the busted version and re-landed a fixed version:
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/6f254d491d00b67c0effe24fbe0b87cbc6f812b0
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/a5ccc32310c9a424ccb4f5106f5155ca20dd360a

Comment 16

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Jared's assessment on manual testing needs (see Comment 12) and the fact that this fix has automated coverage.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8831705 [details]
Bug 1335043 - Prevent the bookmark panel from closing automatically if the mouse is hovering over it.

Annoying UX bug with no known regressions. See comment 12.

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8831705 [details]
Bug 1335043 - Prevent the bookmark panel from closing automatically if the mouse is hovering over it.

fair enough, let's include in 52.1.0esr

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/a49419f75b9c