Closed Bug 1400846 Opened 6 years ago Closed 6 years ago

Bookmark ordering -- folders or BM


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

57 Branch



Firefox 58
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- wontfix
firefox58 --- verified


(Reporter: bawittmeier, Assigned: standard8)



(Keywords: regression, Whiteboard: [fxsearch])


(1 file)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170917100334

Steps to reproduce:

Bookmarks rearranging in Fx57 32bit Nightly

When moving a bookmark or folder from Level2 to Level1 or level3, or Level3 to Level2 or Level1 the moved BM or folder will jump up 1 row in the listing.

Reproducible: always

 . . . Level2
 . . . . . .  Level3

Create a folder at level3 and move it to Level1 or Level2 -- using the dark line placement indicator the moved BM or folder will be inserted 1 row higher than expected.

Actual results:

The moved BM or folder ends up 1 line higher than where the indicator is showing

Expected results:

The moved BM or folder should be inserted where the indicator is showing
Where are you doing the moving, in the bookmarks sidebar, in the library left pane, from the Librart left pane to the right pane (or viceversa)?

Where is the placement indicator shown?

I tried to move a folder I created at level 3 into folder 1 and folder 2, and had no problems doing that, so far.
A video may surely help, but also more specific steps about WHERE and HOW to move the bookmark may help.
Click Bookmarks/Show All Bookmarks from the file menu

In the left column below this is where it occurs.

If I move any folder from a different level (directory wise) when I select the location (dark line indicator) the item is inserted 1 row above where I want.

I use the thick dark line for placement -- I believe this is the intent.


Sorry -- I don't know how to make a video.
Maybe I can reproduce the issue using Bookmarks toolbar.
1. Create a folder on the Bookmarks toolbar.
2. Create a few bookmarks (3) in this folder and 1 subfolder.
3. Create a bookmark in this new subfolder.
4. Move the bookmark from this subfolder to the folder on the Bookmarks toolbar using mouse and try to move it to the bottom, so the placement indicator will be below the last bookmark. 
5. Notice that the bookmark will end up above the last bookmark not below as expected.
Flags: needinfo?(mak77)
[Tracking Requested - why for this release]: Bookmark ordering broken
Ever confirmed: true
Keywords: regression
Component: Untriaged → Bookmarks & History
why should we track this, it's not even clear if it's a 57 regression?
If it's related  to the async transactions work, that is not going to ride the train to 57, regardless.
Just to confirm this is async transactions only, and they'll be turned off for 57. Putting this in my queue as I think I know what caused this.
Assignee: nobody → standard8
Flags: needinfo?(mak77)
Priority: -- → P2
Whiteboard: [fxsearch]
Yes, I can not reproduce the issue if I set browser.places.useAsyncTransactions to false. Currently it is true by default.
I too can confirm it works correctly with browser.places.useAsyncTransactions; false and a restart on the browser.
Comment on attachment 8909769 [details]
Bug 1400846 - Fix ordering of bookmarks with Async Transactions when dropping into different folders.

::: browser/components/places/content/controller.js:1653
(Diff revision 1)
> +          if (existingBookmark && parentGuid == existingBookmark.parentGuid) {
> +            let dragginUp = index < existingBookmark.index;
> +
> -          if (dragginUp) {
> +            if (dragginUp) {
> -            index += movedCount++;
> +              index += movedCount++;
> -          } else if (PlacesUIUtils.useAsyncTransactions) {
> +            } else if (PlacesUIUtils.useAsyncTransactions) {

So, even if it may be correct, I find this if (draggingup) else if (asynctransactions) very confusing.

I'd prefer an explicit 
if (asynctransactions) { ... } else { ... } even if that means duplicating the dragginUp thing inside each brace.
It will also be easier to remove in the future.
Attachment #8909769 - Flags: review?(mak77)
Comment on attachment 8909769 [details]
Bug 1400846 - Fix ordering of bookmarks with Async Transactions when dropping into different folders.

::: browser/components/places/content/controller.js:1664
(Diff revision 2)
> +                continue;
> +              }
> +            } else {
> +              // Sync Transactions.
> +
> +              // Adjust insertion index to prevent reversal of dragged items. When you

too many newlines.
Attachment #8909769 - Flags: review?(mak77) → review+
Pushed by
Fix ordering of bookmarks with Async Transactions when dropping into different folders. r=mak
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
I have verified this bug is fixed with browser.places.useAsyncTransactions set true.

Thank you all for your work.
Please request Beta approval on this when you get a chance.
Flags: needinfo?(standard8)
This only affects async places transactions which we're not shipping in 57.
Flags: needinfo?(standard8)
Build ID: 20171016220427
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.
You need to log in before you can comment on or make changes to this bug.