Last Comment Bug 416459 - Implement proper cut action for bookmarks (was: undo/redo a cut requires two steps, bookmarks are lost if if paste never happens).
: Implement proper cut action for bookmarks (was: undo/redo a cut requires two ...
: dataloss
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
P2 critical (vote)
: Firefox 7
Assigned To: Marco Bonardo [::mak]
: Marco Bonardo [::mak]
: 484578 (view as bug list)
Depends on: 723842 664127 668319 674964
Blocks: 301888 663058 682512
  Show dependency treegraph
Reported: 2008-02-08 16:13 PST by Carsten Book [:Tomcat]
Modified: 2012-03-13 08:59 PDT (History)
10 users (show)
dietrich: blocking‑firefox3.5-
mak77: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v1.0 (23.96 KB, patch)
2011-06-14 07:49 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.1 (30.79 KB, patch)
2011-06-16 18:43 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Test v1.0 (3.55 KB, patch)
2011-06-17 16:31 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.2 (31.14 KB, patch)
2011-06-21 08:06 PDT, Marco Bonardo [::mak]
dietrich: review+
Details | Diff | Splinter Review
screenshot (23.44 KB, image/png)
2011-06-28 08:25 PDT, Marco Bonardo [::mak]
limi: ui‑review+
patch v1.3 (29.16 KB, patch)
2011-06-29 13:46 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review

Description User image Carsten Book [:Tomcat] 2008-02-08 16:13:22 PST
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008020717 Firefox/3.0b4pre (Debug Build)

Steps to reproduce:
- Select a Bookmark and use "cut" from the context menu
- Paste this Bookmark to a folder of your choice
- Select now from the Firefox Menubar Edit->Undo
-> The Bookmark disappears from the bookmark folder where you have pasted him, but is not restored to the original folder
Comment 1 User image Robert [minotaur 11] 2008-04-10 06:46:44 PDT
I was also irritated by this behavior, but i believe it's intended. The first undo only reverts paste, a second undo reverts cut. The Bookmarks Manager in Firefox 2 works identical, but the undo menu entry displays what it will undo.
Comment 2 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2009-02-23 13:24:22 PST
I hit this today while running bfts on WinXP. After a restart I noticed that a whole folder was gone. This is at least a dataloss bug. Users will probably not open the source folder to check if the bookmark/folder has been restored there.

Why we require two steps in undoing an action?
Comment 3 User image Dietrich Ayala (:dietrich) 2009-02-23 15:58:03 PST
This is not a regression, so not blocking 3.1 on it. However I agree that it's not intuitive, and should be fixed to be consistent with the user's expectation of what undo should do.
Comment 4 User image Gervase Markham [:gerv] 2009-11-26 05:52:15 PST
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Comment 5 User image Marco Bonardo [::mak] 2010-06-21 13:55:03 PDT
*** Bug 484578 has been marked as a duplicate of this bug. ***
Comment 6 User image Marco Bonardo [::mak] 2010-06-21 13:56:13 PDT
tentatively assigning, should be fixed sometimes.
Comment 7 User image Marco Bonardo [::mak] 2011-06-14 07:49:02 PDT
Created attachment 539204 [details] [diff] [review]
patch v1.0

Needs a test and styling on trees doesn't work at all, due to the dependent bug (the only workaround that works is setting a border or a background on the cell, but that's not native on any platform).
Apart these, seems to work fine.
Comment 8 User image Marco Bonardo [::mak] 2011-06-14 07:51:01 PDT
A couple advantages of this approach:
- more native
- less disk IO
- less Sync impact
- no unexpected dataloss
- no dataloss for bug 301888
Comment 9 User image Marco Bonardo [::mak] 2011-06-16 18:43:49 PDT
Created attachment 539964 [details] [diff] [review]
patch v1.1

I'm still working on the test, but that can be reviewed apart, in the meanwhile you may start taking a look at the code changes.
The main obstacle (bug 664127) has a patch and a test already.
Comment 10 User image Marco Bonardo [::mak] 2011-06-17 09:53:24 PDT
fwiw, this patch passed a tryserver run.
Comment 11 User image Marco Bonardo [::mak] 2011-06-17 16:31:50 PDT
Created attachment 540178 [details] [diff] [review]
Test v1.0

This test is really simple, but after all, once checked that cut does cut, most of the code is shared with copy (there is a test on copy) and with transactions (various tests), so other cases don't seem that interesting regarding this bug.
If you have special requests, I'll be happy to hear them :)
Comment 12 User image Marco Bonardo [::mak] 2011-06-18 02:36:19 PDT
the test fails on Linux, will have to check what's up there
Comment 13 User image Marco Bonardo [::mak] 2011-06-21 08:06:47 PDT
Created attachment 540755 [details] [diff] [review]
patch v1.2

gtk doesn't like setting a clipboard content without any flavor, so I just added a fancy empty flavor. I've done it for all OSes since this behavior it makes more sense.
Also solved a js warning on shutdown, where a controller was still trying to update a half-dead treeview.
Comment 14 User image Dietrich Ayala (:dietrich) 2011-06-22 10:54:55 PDT
Comment on attachment 540755 [details] [diff] [review]
patch v1.2

Review of attachment 540755 [details] [diff] [review]:

::: browser/components/places/content/controller.js
@@ +1133,5 @@
> +    // Unfortunately just invoking emptyClipboard is not enough, since it
> +    // does not act on the native clipboard.
> +    let xferable = Cc[";1"].
> +                   createInstance(Ci.nsITransferable);
> +    // GTK doesn't like empty transferables, so just add an unknown type.

shouldn't we fix emptyClipboard to do this under GTK, instead of a one-off fix here in Places?
Comment 15 User image Marco Bonardo [::mak] 2011-06-22 13:27:08 PDT
emptyClipboard doesn't empty the clipboard in any OS, it's bad named, it just releases the clipboard owner. So the first part here is needed for all platforms.

The difference is that the next setData call fails on gtk if the transferable doesn't have any flavor, I did not touch that since I don't know what's the expected behavior, failing is not consistent with Win or Mac, but is not wrong by itself (an empty transferable is useless after all). Since any platform may differ regarding that, I took the safest approach.
Comment 16 User image Dietrich Ayala (:dietrich) 2011-06-28 06:49:14 PDT
Comment on attachment 540755 [details] [diff] [review]
patch v1.2

Review of attachment 540755 [details] [diff] [review]:

this looks fine, r=me on the code changes. however, please add a screenshot, and get UI review (just file followups on any polish resulting from the UI review).
Comment 17 User image Marco Bonardo [::mak] 2011-06-28 08:25:18 PDT
Created attachment 542480 [details]

Not extremely noticeable in a static screen but the feedback is evident when cutting, not annoying if you leave something cut in a view. It's mostly the same as implemented in most file managers.
Comment 18 User image Marco Bonardo [::mak] 2011-06-28 15:54:30 PDT
I can give you a trybuild too
Comment 19 User image Alex Limi (:limi) — Firefox UX Team 2011-06-29 09:37:01 PDT
Comment on attachment 542480 [details]

As commented in IRC:

This works great when the site has a favicon, but since we flush favicons pretty often (boo!), it'll be less noticable — can we make the text gray when it isn't selected too? (gray text on blue background doesn't work well, so we can't do it when selected, IMO.

But let's land this initial version, and tweak it later?
Comment 20 User image Marco Bonardo [::mak] 2011-06-29 13:16:55 PDT
(In reply to comment #19)
> This works great when the site has a favicon, but since we flush favicons
> pretty often (boo!), it'll be less noticable — can we make the text gray
> when it isn't selected too?

Right, while in most cases we show an icon (that may be a blank document) I need feedback also when the icon is not visible, since it's possible.
Unfortunately, matching unselected rows in trees is a nightmare (there is no direct matching afaict, and duplicating all basic selected rules would be an overkill), plus not changing text in selected rows would give no feedback when the user selects rows and starts a cut (some other browser vendor thinks this is not a problem, but trying seems completely broken).
I tried using italic (we did for example in the Sync notifications on Win) but it slightly changes size of the buttons in toolbar, jumpy buttons are not a good way.

The best solution I've found so far is adding some opacity to text as well, but less than to the icon, so it stays readable. It has a similar effect to GrayText but blends nicely in case of a selected row. The downside is that this means another opacity fix to the treebody code, but it's feasible.
Comment 21 User image Marco Bonardo [::mak] 2011-06-29 13:46:27 PDT
Created attachment 542927 [details] [diff] [review]
patch v1.3

This patch uses opacity on both icon and text, with text slightly more opaque. needs bug 668319.
Comment 22 User image Marco Bonardo [::mak] 2011-06-30 13:43:12 PDT
Comment 23 User image Marco Bonardo [::mak] 2011-07-01 15:51:34 PDT
Comment 24 User image Simona B [:simonab ] 2011-08-23 06:48:53 PDT
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0

Verified issue on Win XP, Win 7, Ubuntu 11.04 and Mac OS X 10.6 - after "cut" action - undo/redo requires one step and the bookmark is not lost if paste never occurs (it became grayed out) 
I used the following STR:
1. Open Show All Bookmarks
2. Select a Bookmark and use "cut" from the context menu 
3. Paste the Bookmark into a different folder
4. Organize -> Undo/Redo.
Tested using the above STR in the Bookmarks menu, the Bookmarks Toolbar and the Bookmarks sidebar, after "cut" the selected bookmark becomes grayed out (undo/redo is also performed in one step).

Setting resolution to VERIFIED FIXED.

Note You need to log in before you can comment on or make changes to this bug.