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

Status: VERIFIED FIXED

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

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 47, 32bit, ID 20160224030246
STR:
1. Open http://example.org/ in a new tab
2. Press Ctrl+D
3. Press Enter
4. Press Ctrl+D
 [you must perform Steps 2-3 in less than 4 seconds]

AR:  After Step 4 "new bookmark" popup immediately disappears
ER:  Widget should react according to the last interaction
    [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 4)

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).
I can't understand why brand new stuff keeps adding w/o testing. Where's UX team???

Comment 1

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

I can't reproduce this using this page or example.org. I'm on Windows 10, Nightly 48, 2016-04-19.

(In reply to arni2033 from comment #0)
> I can't understand why brand new stuff keeps adding w/o testing. Where's UX
> team???

You should give yourself more credit, you're good at finding bugs. When I fixed bug 1219794 I included some automated tests and there was also manual testing.

Comment 2

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

I followed steps 1-4 very quickly and wasn't able to reproduce this with multiple tries.

Comment 3

[arni2033]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> I followed steps 1-4 very quickly and wasn't able to reproduce this with multiple tries.
I'm sorry, apparently comment 0 wan't precise enough. I got confused by timeouts added in that panel(
I was a bit nervous when writing comment 0, because I already detected a number of bugs of this type.

STR_2_precise:
1. Open http://example.org/ in a new tab, press Ctrl+D,
   click "Remove Bookmark". Don't move mouse pointer  [This step required to set mouse position]
2. Press Ctrl+D
3. Click "Done" (move mouse in a straight line)
4. Press Ctrl+D
5. Move mouse (e.g. move mouse to bookmark's name. You can even click it and start typing)
 [you must perform Steps 2-5 in less than 4 seconds]

AR:    After Step 5 "new bookmark" popup immediately disappears
ER:    Widget should react according to the last interaction

Note:  I don't know exactly the exact set of cases when this happens
> Steps 2-5:   https://dl.dropboxusercontent.com/s/2cocmgqv9gpma0o/bug%201251071%20comment%203.webm

Comment 4

[arni2033]

Yeah. Apparently I accidentally moved mouse by several pixels while performing STR_1.
I can reproduce that effect now. So it's not necessary to even perform clicks.

Comment 5

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Platform triage meeting decision: This is a corner case and we do not feel the need to track this for Fx47.

Comment 6

[Justin Dolske [:Dolske]]

We P-'d this in triage because the STR in comment 3 make this seem like something unlikely a user would encounter.

But looking further, I think what's happening here is that the |mouseover| listener is firing inconsistently when the mouse pointer is already over the panel when it opens. It seems like it only fires (for me, on OS X) when the pointer is over the textfields/expander buttons. (Actually, that seems to include the path those nodes take as we apply the zoom/expand effect to the panel when it opens.)

So here's a clearer STR, that's actually plausible for a real-world user to encounter:

0) Prep: Add a bookmark, draw on your screen where the panel boundary and textfields are. Remove bookmark.

1) Place mouse pointer in that region, but not over a textfield or between the textfield and the star button.

2) Press Control/Command-D to create a bookmark.

3) Optional: move mouse around inside panel and/or click anything except the Folder selector.

4) Panel disappears after 5 seconds.


There might be a DOM or panel bug here, although I suspect the bug is that it fires at all. I'm not sure what the spec says about an element (or page/window) being initially opened in this situation.

In any case, I think an easy workaround would be to just replace the mouseover listener with mousemove instead.

Comment 7

[arni2033]

Wow, I sure am glad that abstract "normal user" now has this problem too. I personally encountered
some variation of this bug the very 1st time I tried to create a bookmark in Nightly. Now I know that
even once this bug is fixed, creating bookmarks will most likely break the workflow due to bug 1219794

The problem is that Firefox breaks every time: (1) delayed/async action is involved (2) developers try
to predict what user wants (w/o even exploring telemetry data, like in regression bug 1219794).
So it's logical to get rid of such dangerous things, but their number keeps growing. And then Marco says that there's not enough resources to work around REAL bugs, e.g. dependency graph of bug 665580.
Proofs:
>(1) bug1197598, bug1251071(this), bug1240632, bug1251864, bug1261133, ~5 bugs I havent reported, etc
>(2) bug1225573, bug1188800 + Co, bug1219510, bug1208208 + bug1208222, ~2 bugs I havent reported, etc

Comment 8

[Lawrence Mandel [:lmandel] (use needinfo)]

As we're now into RC builds, this is wontfix for 47.

Comment 9

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

Attachment: Patch

Arni, are you able to apply this patch and see if it fixes the issue for you?

Comment 10

[arni2033]

Sorry, building the whole thing is too much for me, given I have a lot of not reported bugs and almost no free time. If you need this ASAP, please ask somebody else (but I can test a try-build)

Comment 11

[Dão Gottwald [:dao]]

(In reply to arni2033 from comment #10)
> Sorry, building the whole thing is too much for me, given I have a lot of
> not reported bugs and almost no free time. If you need this ASAP, please ask
> somebody else (but I can test a try-build)

Here's a try build: http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-639425175930861a71374e35f5a9bea7c48efb18/try-win32/

FYI, as of recently there's a new, faster way to build with non-C++ changes: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds

Comment 12

[arni2033]

There're no bugs described in comment 3 and comment 6 in that try build.

Comment 13

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

Thanks arni.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8758382 [details] [diff] [review]
Patch

Unless I'm missing something, you can just call clearTimeout(this._autoCloseTimer) in the popupshown handler instead of the autoCloseId stuff.

What's the change from mouseover to mousemove about? I do think this event makes more sense, but what does this have to do with this bug?

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Side node: in general it's a bad idea to use a Date as an id, since it's not guaranteed to be monotonic (this caused bugs in the past). When you need a unique id, better to just use an empty js object, that should be guaranteed to be unique.

Comment 16

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

(In reply to Dão Gottwald [:dao] from comment #14)
> What's the change from mouseover to mousemove about? I do think this event
> makes more sense, but what does this have to do with this bug?

This bug would still reproduce if the mouse was started inside of an element and then moved within the element.

Comment 17

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

Attachment: Bug 1251071 - "New bookmark" popup disappears if I reopen it after creating bookmark.

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

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8762422 [details]
Bug 1251071 - "New bookmark" popup disappears if I reopen it after creating bookmark.

https://reviewboard.mozilla.org/r/59122/#review56556

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/7410a15b0850
"New bookmark" popup disappears if I reopen it after creating bookmark. r=mak

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for permafailing browser_bookmark_popup.js on OS X:

https://hg.mozilla.org/integration/fx-team/rev/530e96008b7e

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=535a13bd2bf94fcc72d7e2432100fba85f7c322a
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=10013231&repo=fx-team

07:35:33     INFO -  60 INFO Entering test bound panel_shown_for_keyboardshortcut_on_new_bookmark_star_and_autocloses
07:35:33     INFO -  61 INFO TEST-PASS | browser/base/content/test/general/browser_bookmark_popup.js | Page should only be starred prior to popupshown if editing bookmark -
07:35:33     INFO -  62 INFO TEST-PASS | browser/base/content/test/general/browser_bookmark_popup.js | Panel should be 'closed' to start test -
07:35:33     INFO -  63 INFO TEST-PASS | browser/base/content/test/general/browser_bookmark_popup.js | Panel should be 'open' after shownPromise is resolved -
07:35:33     INFO -  64 INFO TEST-PASS | browser/base/content/test/general/browser_bookmark_popup.js | Only one bookmark should exist -
07:35:33     INFO -  65 INFO TEST-PASS | browser/base/content/test/general/browser_bookmark_popup.js | Page is starred -
07:35:33     INFO -  66 INFO TEST-PASS | browser/base/content/test/general/browser_bookmark_popup.js | title should match isEditingBookmark state -
07:35:33     INFO -  67 INFO TEST-PASS | browser/base/content/test/general/browser_bookmark_popup.js | Page is starred after closing -
07:35:33     INFO -  68 INFO TEST-PASS | browser/base/content/test/general/browser_bookmark_popup.js | bookmark should not be present if a panel action should've removed it -
07:35:33     INFO -  69 INFO Leaving test bound panel_shown_for_keyboardshortcut_on_new_bookmark_star_and_autocloses
07:35:33     INFO -  70 INFO Entering test bound panel_shown_for_new_bookmarks_mousemove_mouseout
07:35:33     INFO -  71 INFO TEST-PASS | browser/base/content/test/general/browser_bookmark_popup.js | Page should only be starred prior to popupshown if editing bookmark -
07:35:33     INFO -  72 INFO TEST-PASS | browser/base/content/test/general/browser_bookmark_popup.js | Panel should be 'closed' to start test -
07:35:33     INFO -  73 INFO TEST-PASS | browser/base/content/test/general/browser_bookmark_popup.js | Panel should be 'open' after shownPromise is resolved -
07:35:33     INFO -  74 INFO Waiting for mousemove event
07:35:33     INFO -  75 INFO Got mousemove event
07:35:33     INFO -  76 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bookmark_popup.js | Panel should still be open on mousemove - Got closed, expected open

Comment 21

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

Comment on attachment 8762422 [details]
Bug 1251071 - "New bookmark" popup disappears if I reopen it after creating bookmark.

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

Comment 22

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5741dbe35d3 shows it working now on OSX. Gijs, can you take a look at the interpolateNativeMouseMove function I implemented?

Comment 23

[:Gijs (he/him)]

Comment on attachment 8762422 [details]
Bug 1251071 - "New bookmark" popup disappears if I reopen it after creating bookmark.

https://reviewboard.mozilla.org/r/59122/#review57118

::: testing/mochitest/tests/SimpleTest/EventUtils.js:666
(Diff revision 2)
> +  var x;
> +  var y;
> +  if (x0 == x1) {
> +    for (y = y0;
> +         y0 < y1 ? y <= y1 : y >= y1;
> +         y += y0 < y1 ? 3 : -3) {

These increments/decrements look like they'll break if the difference between the coordinates isn't a multiple of 3. Is that just me misreading the code?

I'm not really sure why we need this code, either, so an explanation would be useful. Last but not least, I think Neil Deakin would be a better reviewer for this than I am.

Comment 24

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

Comment on attachment 8762422 [details]
Bug 1251071 - "New bookmark" popup disappears if I reopen it after creating bookmark.

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

Comment 25

[Neil Deakin]

Why do you need to use native mouse synthesis here? You should be able to test the popup hiding/not hiding on mouse events behaviour by just using synthesizeMouse.

Comment 26

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

Comment on attachment 8762422 [details]
Bug 1251071 - "New bookmark" popup disappears if I reopen it after creating bookmark.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59122/diff/3-4/

Comment 27

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

Comment on attachment 8762422 [details]
Bug 1251071 - "New bookmark" popup disappears if I reopen it after creating bookmark.

This version now uses EventUtils.synthesizeMouse. I wanted to pass it back to you to review it again since it's a bit different, though simpler.

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8762422 [details]
Bug 1251071 - "New bookmark" popup disappears if I reopen it after creating bookmark.

https://reviewboard.mozilla.org/r/59122/#review59372

::: browser/base/content/test/general/browser_bookmark_popup.js:132
(Diff revision 4)
>      popupShowFn() {
>        bookmarkStar.click();
>      },
>      *popupEditFn() {
> -      let mouseOverPromise = new Promise(resolve => {
> -        bookmarkPanel.addEventListener("mouseover", function onmouseover() {
> +      let mouseMovePromise = BrowserTestUtils.waitForEvent(bookmarkPanel, "mousemove");
> +      EventUtils.synthesizeMouse(bookmarkPanel, 0, 0, {type: "mousemove"});

If there's no specific reason to use 0,0, I'd say you could even just use synthesizeMouseAtCenter?

Comment 29

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

Attachment: Patch for check-in

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/88038ba05ff1
"New bookmark" popup disappears if I reopen it after creating bookmark. r=mak

Comment 31

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

https://hg.mozilla.org/mozilla-central/rev/88038ba05ff1

Comment 32

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

jaws, what do you think about uplift for this patch? It is late to bring it to beta, but aurora could be ok if you don't think it is likely to cause other regressions.

Comment 33

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

Comment on attachment 8768405 [details] [diff] [review]
Patch for check-in

Approval Request Comment
[Feature/regressing bug #]: bug 1219794
[User impact if declined]: the bookmark panel can close unexpectedly when creating a new bookmark
[Describe test coverage new/current, TreeHerder]: automated testing, been on m-c for a week.
[Risks and why]: low risk, only affects bookmark panel, but has automated testing and manual testing
[String/UUID change made/needed]: none

Comment 34

[Hossain Al Ikram [:ikram] (QA Contact)]

I have reproduced this Bug with Nightly 47.0a1 (2016-02-24) on Windows 7, 64 Bit!

The Bug's fix is now verified on Latest Nightly 50.0a1.

Build ID 	20160722030235
User Agent 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0

Comment 35

[Gerry Chang [:gchang]]

Comment on attachment 8768405 [details] [diff] [review]
Patch for check-in

Review of attachment 8768405 [details] [diff] [review]:
-----------------------------------------------------------------

This patch fixes a regression. Let's take it in aurora.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee476b2b7d46