674732 - Cannot duplicate tab via holding CTRL key

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Pavel Cvrcek [:JasnaPaka]]

Bug 455694 was landed and after that I cannot duplicate tab via holding CTRL key.

How to reproduce:
* Press key CTRL and try to move tab.

Expected behavior:
* Tab is duplicated.

Current behavior:
* Tab is moved.

Comment 1

[Frank Yan (:fryn)]

When dragging the tab, you are applying direct manipulation to it, analogous to dragging a window, so duplicating them with a drag doesn't make sense.

Hold ctrl/cmd when clicking the reload button to duplicate tabs.

Comment 2

[Alice0775 White]

Duplicating a tab to the another browser window is now broken due to landing Bug 455694.

[STR]
1. Open 2 browser window and several tabs.
2. Press CTRL key and drag a tab to the another browser window's tab strip. and drop.

[Actrual]
The dragged tab is moved.

[Expected]
The dragged tab should be duplicated.

Comment 3

[Frank Yan (:fryn)]

(In reply to comment #2)

As I explained above, direct manipulation is a much bigger UX win, and it is not worth muddling that conceptual model by supporting this use case.

Comment 4

[Dão Gottwald [:dao]]

We can sort-of support this as well as bookmarking by initiation a traditional drag (e.g. with an HTML link flavor) when holding CTRL, right? No detach animation needed in that case.

Comment 5

[Lozzy]

(In reply to comment #1)
> When dragging the tab, you are applying direct manipulation to it, analogous
> to dragging a window, so duplicating them with a drag doesn't make sense.
> 
> Hold ctrl/cmd when clicking the reload button to duplicate tabs.

I feel like you're breaking a known usage scenario which people are already trained to and providing what I feel is a rather poor reason. To add to that, the solution you're suggesting is even more nonsensical. It wouldn't even appear that you've done testing to ascertain the impact of the change.

I'd like to think that you're going to keep the old functionality in the browser - selectable via a pref - because I don't feel that having prettier tab drag previews justifies the bugs and regressions introduced. I know I will sorely moss it otherwise.

Comment 6

[Frank Yan (:fryn)]

(In reply to comment #4)
> We can sort-of support this as well as bookmarking by initiation a
> traditional drag (e.g. with an HTML link flavor) when holding CTRL, right?
> No detach animation needed in that case.

True; it will require that ctrl (opt on os x) be held at the beginning of the drag.

Comment 7

[Dão Gottwald [:dao]]

I'm not sure this ever worked when you started pressing CTRL in the middle of the drag operation. I certainly didn't expected.

Comment 8

[Dão Gottwald [:dao]]

Erm, I certainly didn't expect it to work this way.

Comment 9

[Lozzy]

(In reply to comment #7 & #8)

It's how I've always done it. CTRL at the start of the drag isn't ideal, but an acceptable compromise.

In reply to comment #4)
> We can sort-of support this as well as bookmarking by initiation a
> traditional drag (e.g. with an HTML link flavor) when holding CTRL, right?
> No detach animation needed in that case.

By this, do you mean that the tab will actually be cloned, or do you mean that the URL of the tab will be opened in a new tab? The former is definitely preferable.

Comment 10

[Frank Yan (:fryn)]

Attachment: patch

I'm using text/x-moz-url here.
I can add text/uri-list and/or text/plain too, but I doubt their usefulness.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 549076 [details] [diff] [review]
patch

>+          dt.mozCursor = "default"; // Set the cursor to an arrow.

This used to be done for tab detaching, which this new code path explicitly doesn't cover. Seems like the standard drag cursor should be used here.

>+          dt.setDragImage(tabPreviews.capture(tab), 0, 0);

I'm not sure this is useful in this context either...

Comment 12

[Frank Yan (:fryn)]

Attachment: patch v2

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 549086 [details] [diff] [review]
patch v2

>+          let uri = this.tabbrowser.getBrowserForTab(tab).currentURI;
>+          let spec = uri ? uri.spec : "about:blank";

Not sure why currentURI would be null here... seems like this should never be the case.

Comment 14

[Dão Gottwald [:dao]]

Well, I guess it might be the case when you're creating a new browser, but it shouldn't be the case /here/.

Comment 15

[Frank Yan (:fryn)]

(In reply to comment #13)
> Comment on attachment 549086 [details] [diff] [review] [review]
> patch v2
> 
> >+          let uri = this.tabbrowser.getBrowserForTab(tab).currentURI;
> >+          let spec = uri ? uri.spec : "about:blank";
> 
> Not sure why currentURI would be null here... seems like this should never
> be the case.

(In reply to comment #14)
> Well, I guess it might be the case when you're creating a new browser, but
> it shouldn't be the case /here/.

I took it from the pre-animation d&d code:
https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3333
> <handler event="dragstart"><![CDATA[
> var tab = this._getDragTargetTab(event);
> if (!tab)
>   return;
> 
> let dt = event.dataTransfer;
> dt.mozSetDataAt(TAB_DROP_TYPE, tab, 0);
> let uri = this.tabbrowser.getBrowserForTab(tab).currentURI;
> let spec = uri ? uri.spec : "about:blank";

The Places controller used similar code:
https://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/controller.js#1529
> else if (data instanceof XULElement && data.localName == "tab" &&
>          data.ownerDocument.defaultView instanceof ChromeWindow) {
>   let uri = data.linkedBrowser.currentURI;
>   let spec = uri ? uri.spec : "about:blank";

But the private browsing code makes a non-null assumption:
https://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#185

I'm not sure exactly where currentURI is initialized, so I'll leave it as is for now.
I can change it if it can be guaranteed to be non-null.

Comment 16

[Dão Gottwald [:dao]]

I suspect lots of stuff would break if it could be null for a browser that's actually in use (i.e. not a browser that's being created or destroyed).

Comment 17

[Dão Gottwald [:dao]]

> I took it from the pre-animation d&d code:
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#3333

> The Places controller used similar code:
> https://mxr.mozilla.org/mozilla-central/source/browser/components/places/
> content/controller.js#1529

The sad truth is probably that this existed in just one place, likely an ancient one, and was copied over without people understanding it. Bad or even broken code can live on forever :(

Comment 18

[Frank Yan (:fryn)]

Pushed to m-c instead of fx-team to get this in the next nightly to avoid superfluous duplicate bugs.

https://hg.mozilla.org/mozilla-central/rev/5fc7b3f0bae6
https://hg.mozilla.org/mozilla-central/rev/cef1817c3b13

Comment 19

[Frank Yan (:fryn)]

I removed the null check as per Dão's comment, with which I agree.

Comment 20

[Frank Yan (:fryn)]

(In reply to comment #4)
> We can sort-of support this as well as bookmarking by initiation a
> traditional drag (e.g. with an HTML link flavor) when holding CTRL, right?
> No detach animation needed in that case.

Also, thanks Dão for coming up with this.

Comment 21

[Alice0775 White]

I think this fix is not "duplicate tab" just reopen URL of the dragged tab.
The copy does not have the same session history and plugin state of the original tab.

Is this intentional?
If so I will file a bug.

Comment 22

[Alice0775 White]

Error, the 2nd line should read as follows.
The copy does not have the same session history and scroll position of the original tab.

Comment 23

[Dão Gottwald [:dao]]

(In reply to comment #21)
> I think this fix is not "duplicate tab" just reopen URL of the dragged tab.
> The copy does not have the same session history and plugin state of the
> original tab.
> 
> Is this intentional?

Yes.

> If so I will file a bug.

You can file a bug on it, but I think the current compromise is all we'll do for now.

Comment 24

[(mostly gone) XtC4UaLL [:xtc4uall]]

(In reply to comment #23)
> You can file a bug on it, but I think the current compromise is all we'll do
> for now.

Would backing this out (since different Duplication Behavior available) and rather Reconsideration of Bug 455722/Bug 594217 (since ordinary Duplication Behavior available) be an Option?

Comment 25

[Dão Gottwald [:dao]]

No.

Comment 26

[Lozzy]

Will the old drag and drop code still be in the browser? If so I'd much prefer to use that; as I said, prettier animations don't justify the myriad of bugs and regressions this is introducing.

And this compromise isn't worthwhile, it's just not tab duplication.

Comment 27

[Joe Wilson]

Should I open a new bug about the fix in this bug (it is only partial, in my opinion)? Now you can't start dragging tab first and THEN hit control. The tab will just detach in that case, though the ctrl is pressed at the moment you "drop" tab.
It works well in any Windows' Explorer: you may hit CTRL later, at the moment you are going to "drop" the selected items.

Comment 28

[Lozzy]

I think you should Joe. Hopefully once the next release train starts this will be able to be looked at more closely. I get the feeling that there is pressure to get a visible, user facing feature into the current release, hence why the new tab drag is being pushed out of the door in an unsatisfactory state.

That's just my theory, but hopefully if you file a new bug for it someone can get around to giving the feature polish in future.

Comment 29

[Joe Wilson]

Done.
Bug 675438

Comment 30

[Daniel Desira]

verified [testday-20110930]

Comment 31

[Daniel Desira]

verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20100101 Firefox/8.0

Comment 32

[Teodosia Pop]

Verified fixed using Mozilla/5.0 (Windows NT 6.1; rv:8.0) Gecko/20100101 Firefox/8.0.