Cannot duplicate tab via holding CTRL key

VERIFIED FIXED in Firefox 8

Status

()

Firefox
Tabbed Browser
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: JasnaPaka, Assigned: fryn)

Tracking

({regression, verified-beta})

Trunk
Firefox 8
regression, verified-beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [testday-20110930])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
No longer blocks: 455694

Updated

6 years ago
Blocks: 455694
(Assignee)

Comment 1

6 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX

Updated

6 years ago
Keywords: regression

Comment 2

6 years ago
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Comment 3

6 years ago
(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.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → WONTFIX
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

6 years ago
(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.

Updated

6 years ago
Blocks: 674723
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Comment 6

6 years ago
(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.
I'm not sure this ever worked when you started pressing CTRL in the middle of the drag operation. I certainly didn't expected.
Erm, I certainly didn't expect it to work this way.

Comment 9

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

Comment 10

6 years ago
Created attachment 549076 [details] [diff] [review]
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.
Assignee: nobody → fryn
Status: REOPENED → ASSIGNED
Attachment #549076 - Flags: review?(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...
Attachment #549076 - Flags: review?(dao) → review-
(Assignee)

Comment 12

6 years ago
Created attachment 549086 [details] [diff] [review]
patch v2
Attachment #549076 - Attachment is obsolete: true
Attachment #549086 - Flags: review?(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.
Attachment #549086 - Flags: review?(dao) → review+
Well, I guess it might be the case when you're creating a new browser, but it shouldn't be the case /here/.
(Assignee)

Comment 15

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

Comment 18

6 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
Version: unspecified → Trunk
(Assignee)

Comment 19

6 years ago
I removed the null check as per Dão's comment, with which I agree.
(Assignee)

Comment 20

6 years ago
(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

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

6 years ago
Error, the 2nd line should read as follows.
The copy does not have the same session history and scroll position of the original tab.
(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.
(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?
No.

Comment 26

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

Updated

6 years ago
Depends on: 675215

Comment 27

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

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

Updated

6 years ago
Blocks: 675438

Comment 29

6 years ago
Done.
Bug 675438

Updated

6 years ago
Blocks: 675444

Comment 30

6 years ago
verified [testday-20110930]

Comment 31

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

Comment 32

6 years ago
Verified fixed using Mozilla/5.0 (Windows NT 6.1; rv:8.0) Gecko/20100101 Firefox/8.0.
Status: RESOLVED → VERIFIED
Keywords: verified-beta

Updated

6 years ago
Whiteboard: [testday-2011030]

Updated

6 years ago
Whiteboard: [testday-2011030] → [testday-20110930]
You need to log in before you can comment on or make changes to this bug.