Reordering of Bookmarks via Drag&Drop is incorrect

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Bookmarks & History
P1
normal
VERIFIED FIXED
3 months ago
a month ago

People

(Reporter: Alice0775 White, Assigned: standard8)

Tracking

({regression})

Trunk
Firefox 57
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 disabled, firefox57 fixed, firefox58 verified)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
@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

Updated

3 months ago
Priority: -- → P1
Whiteboard: [fxsearch]

Updated

3 months ago
Blocks: 979280
(Assignee)

Updated

3 months ago
Assignee: nobody → standard8
Comment hidden (mozreview-request)
(Assignee)

Comment 2

3 months ago
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

3 months ago
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

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

Updated

3 months ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

3 months ago
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.
Attachment #8900222 - Flags: review?(mak77)
Comment hidden (mozreview-request)

Comment 7

3 months ago
mozreview-review
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.
Attachment #8900222 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 9

3 months ago
(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

3 months ago
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 hidden (mozreview-request)

Comment 12

3 months ago
Pushed by mbanner@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/7d7e8ffee135
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1395784
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]
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
QA Whiteboard: [bugday-20170913]

Comment 16

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

Build ID: faa897d7948b7e2439573f39c34366c138913663
(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

2 months ago
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.
(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.
(Reporter)

Updated

2 months ago
Depends on: 1400846

Comment 20

a month ago
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.
Status: RESOLVED → VERIFIED
status-firefox58: --- → verified
You need to log in before you can comment on or make changes to this bug.