Bookmark ordering -- folders or BM

VERIFIED FIXED in Firefox 58

Status

()

defect
P2
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: bawittmeier, Assigned: standard8)

Tracking

({regression})

57 Branch
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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

STR:
Level1
 . . . 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.
(Reporter)

Comment 2

2 years ago
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.

Image:
[img]https://i.imgur.com/STE21k2.jpg[/img]

Sorry -- I don't know how to make a video.

Comment 3

2 years ago
Maybe I can reproduce the issue using Bookmarks toolbar.
STR:
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.

Updated

2 years ago
Flags: needinfo?(mak77)

Comment 4

2 years ago
[Tracking Requested - why for this release]: Bookmark ordering broken
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression

Updated

2 years ago
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.
(Assignee)

Comment 7

2 years ago
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]

Comment 8

2 years ago
Yes, I can not reproduce the issue if I set browser.places.useAsyncTransactions to false. Currently it is true by default.
(Reporter)

Comment 9

2 years ago
I too can confirm it works correctly with browser.places.useAsyncTransactions; false and a restart on the browser.

Comment 11

2 years ago
mozreview-review
Comment on attachment 8909769 [details]
Bug 1400846 - Fix ordering of bookmarks with Async Transactions when dropping into different folders.

https://reviewboard.mozilla.org/r/181268/#review187076

::: 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 hidden (mozreview-request)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8909769 [details]
Bug 1400846 - Fix ordering of bookmarks with Async Transactions when dropping into different folders.

https://reviewboard.mozilla.org/r/181268/#review187482

::: 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+
Comment hidden (mozreview-request)

Comment 15

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/96757433b36d
Fix ordering of bookmarks with Async Transactions when dropping into different folders. r=mak
https://hg.mozilla.org/mozilla-central/rev/96757433b36d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
(Reporter)

Comment 17

2 years ago
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)
(Assignee)

Comment 19

2 years ago
This only affects async places transactions which we're not shipping in 57.
Flags: needinfo?(standard8)

Comment 20

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