1391166 - Reordering of Bookmarks via Drag&Drop is incorrect

Status: VERIFIED FIXED

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

Description

[Alice0775 White]

@dsmith reported a bug, see http://forums.mozillazine.org/viewtopic.php?p=14761577#p14761577


I'm seeing something strange in bookmarks ordering.

Reproducible: always

Steps to Reproduce:
Given bookmarks (in folder on the bookmarks toolbar):

A
B
C

If I drag A *down* to be above C, 

(Expected Results)
I expect to get: 

B
A
C

(Actual Results)
But instead I get:

B
C
A

However I can drag A back *up* the list and it will work. For example, moving A above C properly gives me:

B
A
C

並び替えは正しくない結果になる

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0da00124af43d44fed96133300ba5e32b0d821a8&tochange=0a4690dfd7b383e2f732210cf8906ce51a5b2433

Regressed by:0a4690dfd7b3	Mark Banner — Bug 1071513 - Enable async PlacesTransactions for nightly builds. r=mak

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1391166 - Fix the order of bookmarks via drag and drop when moving a bookmark down in the list.

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

Comment 2

[Mark Banner (:standard8)]

The patch adds in a bit of code that is handled in the old transactions but isn't in the new ones:

http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/toolkit/components/places/nsNavBookmarks.cpp#1386

Comment 3

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

IIRC the reason the new code doesn't act like the old one, is that I didn't want to call update with index: 1 and have the bookmark end up at index: 2. Basically, I wanted the final situation to respect what was passed to update().
That means it's the consumer that must do its own calculations.
What do you think?

Comment 4

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

well, actually it was the opposite, update with index: 2 and the bookmark ends up at index 1.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8900222 [details]
Bug 1391166 - Fix the order of bookmarks via drag and drop when moving a bookmark down in the list.

I'm happy with the other approach of making the caller change. I was kinda split and went with one, but I'm happy to swap this over, the logic makes sense.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8900222 [details]
Bug 1391166 - Fix the order of bookmarks via drag and drop when moving a bookmark down in the list.

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

Comment 7

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

Comment on attachment 8900222 [details]
Bug 1391166 - Fix the order of bookmarks via drag and drop when moving a bookmark down in the list.

https://reviewboard.mozilla.org/r/171590/#review177062

There's still a problem, try draggin up a bookmark just above itself (so the drop indicator appears where it is already, we seem to still subtract 1 and move the bookmark up by 1 position, while it should not move.
Though, it seems to happen also when disabling Async Transactions, so probably it's just a miscalculation of the insertion point. Worth filing a follow-up, but won't block this since it's not an async transactions regression.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8900222 [details]
Bug 1391166 - Fix the order of bookmarks via drag and drop when moving a bookmark down in the list.

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

Comment 9

[Mark Banner (:standard8)]

(In reply to Marco Bonardo [::mak] from comment #7)
> There's still a problem, try draggin up a bookmark just above itself (so the
> drop indicator appears where it is already, we seem to still subtract 1 and
> move the bookmark up by 1 position, while it should not move.
> Though, it seems to happen also when disabling Async Transactions, so
> probably it's just a miscalculation of the insertion point. Worth filing a
> follow-up, but won't block this since it's not an async transactions
> regression.

It turned out this was async transactions only. I've added an additional check for when the indexes are the same, we won't add the transaction onto the queue, and hence we won't do a batch operation if we don't need to.

Comment 10

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 801900a6e2f5 -d 8cb237283ed0: rebasing 416840:801900a6e2f5 "Bug 1391166 - Fix the order of bookmarks via drag and drop when moving a bookmark down in the list. r=mak" (tip)
merging browser/components/places/tests/browser/browser.ini
warning: conflicts while merging browser/components/places/tests/browser/browser.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8900222 [details]
Bug 1391166 - Fix the order of bookmarks via drag and drop when moving a bookmark down in the list.

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d7e8ffee135
Fix the order of bookmarks via drag and drop when moving a bookmark down in the list. r=mak

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/7d7e8ffee135

Comment 14

[Almas Hossain[:mAlmas]]

I have reproduced this bug with Nightly 57.0a1 (2017-08-16) in Windows 10 (64-bit).

This bug's fix is verified with latest Nightly 57.0a1 (64-bit).

Build ID   :   20170906100107
User Agent :   Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday 20170906]

Comment 15

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 56.0a1 (2017-07-27) (64-bit) on Ubuntu 16.04 LTS! 

This bug's fix is Verified with latest Nightly!

Build ID   :  20170913100125
User Agent :  Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 16

[Bruce]

This bug is marked verified Fixed.  However, it is not fixed in the latest 32 bit Nightly.

Build ID: faa897d7948b7e2439573f39c34366c138913663

Comment 17

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

(In reply to Bruce from comment #16)
> This bug is marked verified Fixed.  However, it is not fixed in the latest
> 32 bit Nightly.

What are your steps to reproduce the bug? I just tried and it WFM.

Comment 18

[Bruce]

Sorry I failed to indicate the str.

Folder structure:
Level1
...Level2
......Level3

Moving from any same folder level it works fine.

If moving a folder or BM from Level3 to level1 or level2 it may not land where you want it.

Comment 19

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

(In reply to Bruce from comment #18)
> If moving a folder or BM from Level3 to level1 or level2 it may not land
> where you want it.

it looks like a different bug from comment 0, please file it as a separate bug so we can handle it properly.

Comment 20

[Hani Yacoub, Desktop QA]

Build ID: 20171017220415
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0

Verified as fixed on Firefox Nightly 58.0a1 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.