Closed
Bug 1391166
Opened 7 years ago
Closed 7 years ago
Reordering of Bookmarks via Drag&Drop is incorrect
Categories
(Firefox :: Bookmarks & History, defect, P1)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | disabled |
firefox57 | --- | fixed |
firefox58 | --- | verified |
People
(Reporter: alice0775, Assigned: standard8)
References
Details
(Keywords: regression, Whiteboard: [fxsearch])
Attachments
(1 file)
@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•7 years ago
|
Priority: -- → P1
Whiteboard: [fxsearch]
Updated•7 years ago
|
Blocks: PlacesAsyncTransact
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → standard8
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years 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•7 years 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•7 years ago
|
||
well, actually it was the opposite, update with index: 2 and the bookmark ends up at index 1.
Updated•7 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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
Comment 13•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 14•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
This bug is marked verified Fixed. However, it is not fixed in the latest 32 bit Nightly.
Build ID: faa897d7948b7e2439573f39c34366c138913663
Comment 17•7 years ago
|
||
(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•7 years 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.
Comment 19•7 years ago
|
||
(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•7 years 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
Updated•6 years ago
|
Flags: in-qa-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•