Closed Bug 1458049 Opened 6 years ago Closed 6 years ago

Implement ability to move a selection of tabs into a new window through tab context menu

Categories

(Firefox :: Tabbed Browser, enhancement, P3)

Product:
Firefox
Component:
Tabbed Browser
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed
firefox64 --- verified
firefox65 --- verified

People

(Reporter: ablayelyfondou, Assigned: ablayelyfondou, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: feature)

Attachments

(1 file)

Bug 1458049 - Implement ability to move a selection of tabs into a new window through tab context menu.
59 bytes, text/x-review-board-request
jaws
: review+
Details 
When a group of tabs are selected,right-clicking on any tab in the selection should provide a context menu option of "Move (selected tabs) to new window".

We will need more information here from the design team about the exact title to use. Because "Move selected tabs to new windows" could be too long and keeping it to "Move to new window" (current title for a single tab) might be confusing.
Blocks: tabs-multiselect
We could also use "Move N tabs to new window", where N is replaced by the number of tabs.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Yes! That sounds good to me.
Does this feature affect to the WebExtensions API: browser.tabs.move()? When one of multiselected tab is specified, all multiselected tabs should be moved by the API?
Extensions would need to use browser.tabs.query({highlighted: true}) and can then use the results of the query with browser.tabs.move().
Comment on attachment 8992014 [details]
Bug 1458049 - Implement ability to move a selection of tabs into a new window through tab context menu.

https://reviewboard.mozilla.org/r/255716/#review263788

::: browser/base/content/tabbrowser.js:3472
(Diff revision 1)
> +      for (let i = 1; i < noActiveTabs.length; i++) {
> +        win.gBrowser.adoptTab(noActiveTabs[i], i);
> +      }
> +
> +      if (activeTabNewIndex > -1) {
> +        win.gBrowser.adoptTab(activeTab, activeTabNewIndex, false);

I forgot that I modified here a bit (for debug purpose). We actually should have true instead of false for the last argument to keep the previously active tab showing. But then the error will show up two times.
Comment on attachment 8992014 [details]
Bug 1458049 - Implement ability to move a selection of tabs into a new window through tab context menu.

https://reviewboard.mozilla.org/r/256896/#review264044

Looks good, but I've highlighted some changes I think should be made.

::: browser/base/content/tabbrowser.js:3432
(Diff revision 3)
>  
> +  /**
> +   * Move given tabs in param to a new browser window, unless they are already the only tabs
> +   * in the current window, in which case this will do nothing.
> +   */
> +  replaceTabsWithWindow(tabs) {

I would prefer combining selectedTabsInVisualOrder, moveToNewWindowToggled, and replaceTabsWithWindow.

replaceTabsWithWindow would just take the contextTab as its argument, then if the contextTab is multiselected it will use the Array.filter (described below), otherwise it will just call replaceTabWithWindow.

How's something like this:
```js
replaceTabsWithWindow(contextTab) {
  let tabs;
  if (contextTab.multiselected) {
    tabs = this.selectedTabs;
  } else {
    tabs = [gBrowser.selectedTab];
  }
  
  if (this.tabs.length == tabs.length) {
    return null;
  }
  
  if (tabs.length == 1) {
    return this.replaceTabWithWindow(tabs[0]);
  }
  
  let inactiveTabs = tabs.filter(t => t != gBrowser.selectedTab);
  let activeTabNewIndex = tabs.indexOf(gBrowser.selectedTab);
  
  // The rest of the function looks okay from here.
```

::: browser/base/content/tabbrowser.js:3445
(Diff revision 3)
> +
> +    // The order of the tabs is preserved.
> +    // To avoid mutliple tab-switch, the active tab is "moved" lastly, if applicable.
> +    // If applicable, the active tab remains active in the new window.
> +    let noActiveTabs = [];
> +    let activeTab =  gBrowser.selectedTab;

nit, only one space after the = sign.

::: browser/base/content/tabbrowser.js:3472
(Diff revision 3)
> +      for (let i = 1; i < noActiveTabs.length; i++) {
> +        win.gBrowser.adoptTab(noActiveTabs[i], i);
> +      }
> +
> +      if (activeTabNewIndex > -1) {
> +      win.gBrowser.adoptTab(activeTab, activeTabNewIndex, true /* aSelectTab */);

nit, indentation.

::: browser/base/content/tabbrowser.js:3475
(Diff revision 3)
> +      var evt = document.createEvent("UIEvents");
> +      evt.initUIEvent("TabsMoved", true, false, win, tabs.length);
> +      window.dispatchEvent(evt);

This event is only used by the test. I think we should remove it and update the test to just wait for all tabs to be moved over.

::: browser/base/content/tabbrowser.js:3755
(Diff revision 3)
>      let tabs = ChromeUtils.nondeterministicGetWeakSetKeys(_multiSelectedTabsSet)
>        .filter(tab => tab.isConnected && !tab.closing);
>      if (!_multiSelectedTabsSet.has(selectedTab)) {
>        tabs.push(selectedTab);
>      }
>      return tabs;

The selectedTabs getter should probably return the tabs in visual order.

At the last step of this getter, you can do `return tabs.sort((a, b) => a._tPos > b._tPos);`

::: browser/base/content/test/tabs/browser_multiselect_tabs_move_to_new_window_contextmenu.js:27
(Diff revision 3)
> +    let tabsMoved = BrowserTestUtils.waitForEvent(window, "TabsMoved");
> +    let newWindow = gBrowser.moveToNewWindowToggled(tab1);
> +    await tabsMoved;

You can replace the TabsMoved with:

`TestUtils.waitForCondition(() => newWindow.gBrowser.visibleTabs.length == 3, "Wait for all three tabs to get moved to the new window");`

::: browser/base/content/test/tabs/browser_multiselect_tabs_move_to_new_window_contextmenu.js:37
(Diff revision 3)
> +
> +    is(gBrowser.multiSelectedTabsCount, 0, "Zero multiselected tabs");
> +    is(gBrowser.visibleTabs.length, 2, "Two tabs now in the old window");
> +    is(gBrowser2.visibleTabs.length, 3, "Three tabs in the new window");
> +    is(gBrowser2.visibleTabs.indexOf(gBrowser2.selectedTab), 1,
> +     "Previous active tab has still the focus");

"Previously active tab is still the active tab in the new window"
Attachment #8992014 - Flags: review?(jaws) → review-
Comment on attachment 8992014 [details]
Bug 1458049 - Implement ability to move a selection of tabs into a new window through tab context menu.

https://reviewboard.mozilla.org/r/256896/#review264044

> I would prefer combining selectedTabsInVisualOrder, moveToNewWindowToggled, and replaceTabsWithWindow.
> 
> replaceTabsWithWindow would just take the contextTab as its argument, then if the contextTab is multiselected it will use the Array.filter (described below), otherwise it will just call replaceTabWithWindow.
> 
> How's something like this:
> ```js
> replaceTabsWithWindow(contextTab) {
>   let tabs;
>   if (contextTab.multiselected) {
>     tabs = this.selectedTabs;
>   } else {
>     tabs = [gBrowser.selectedTab];
>   }
>   
>   if (this.tabs.length == tabs.length) {
>     return null;
>   }
>   
>   if (tabs.length == 1) {
>     return this.replaceTabWithWindow(tabs[0]);
>   }
>   
>   let inactiveTabs = tabs.filter(t => t != gBrowser.selectedTab);
>   let activeTabNewIndex = tabs.indexOf(gBrowser.selectedTab);
>   
>   // The rest of the function looks okay from here.
> ```

I agree with this except that I think you didn't use selectedTabsInVisualOrder instead of selectedTabs which gives a list of tabs in the order they were selected. It's important to get a list of tabs in the order they appear because we need to restore it in the new window. What do you think?
Comment on attachment 8992014 [details]
Bug 1458049 - Implement ability to move a selection of tabs into a new window through tab context menu.

https://reviewboard.mozilla.org/r/256896/#review264044

> The selectedTabs getter should probably return the tabs in visual order.
> 
> At the last step of this getter, you can do `return tabs.sort((a, b) => a._tPos > b._tPos);`

Yes, I see why you used selectedTabs in replaceTabsWithWindow now. So my above comment should be obsolete.
Comment on attachment 8992014 [details]
Bug 1458049 - Implement ability to move a selection of tabs into a new window through tab context menu.

https://reviewboard.mozilla.org/r/256896/#review264044

> I agree with this except that I think you didn't use selectedTabsInVisualOrder instead of selectedTabs which gives a list of tabs in the order they were selected. It's important to get a list of tabs in the order they appear because we need to restore it in the new window. What do you think?

This comment is obsolete now, see bellow.
Comment on attachment 8992014 [details]
Bug 1458049 - Implement ability to move a selection of tabs into a new window through tab context menu.

https://reviewboard.mozilla.org/r/256896/#review264044

Hey Jared, I think it works great now. Instead of "SwapDocShells", I should wait for the event "EndSwapDocShells" in "replaceTabsWithWindow" function.
Comment on attachment 8992014 [details]
Bug 1458049 - Implement ability to move a selection of tabs into a new window through tab context menu.

https://reviewboard.mozilla.org/r/256896/#review264570

::: browser/base/content/test/tabs/browser_multiselect_tabs_move_to_new_window_contextmenu.js:34
(Diff revisions 3 - 4)
> -    await tabsMoved;
> +  // waiting for tab2 to close ensure that the newWindow is created,
> +  // thus newWindow.gBrowser used in the second waitForCondition
> +  // will not be undefined.
> +  await TestUtils.waitForCondition(() => tab2.closing, "Wait for tab2 to close");
> +  await TestUtils.waitForCondition(() => newWindow.gBrowser.visibleTabs.length == 3,
> +  "Wait for all three tabs to get moved to the new window");

Can you please indent this line two spaces like is done for line 42?
Attachment #8992014 - Flags: review?(jaws) → review+
Comment on attachment 8992014 [details]
Bug 1458049 - Implement ability to move a selection of tabs into a new window through tab context menu.

https://reviewboard.mozilla.org/r/256896/#review264572

::: browser/base/content/tabbrowser.js:3749
(Diff revision 4)
> +  },
> +
> +  /**
> +   * Return a list of selected Tabs in the order they appear in the tab-browser.
> +   */
> +  get selectedTabsInVisualOrder() {

This function is unused and should be deleted.
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/61a9c1585620
Implement ability to move a selection of tabs into a new window through tab context menu. r=jaws
https://hg.mozilla.org/mozilla-central/rev/61a9c1585620
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Jared, Is that something that something we want to add to the release notes?
Assignee: nobody → ablayelyfondou
status-firefox61: affected → ---
Flags: needinfo?(jaws)
Keywords: feature
Depends on: 1477152
This is part of a larger task which is already planning a release note. Bug 1474938 tracks enabling the feature for beta and release builds.
Flags: needinfo?(jaws)
Depends on: 1477724
No longer depends on: 1477152
Depends on: 629232
Verified fixed that this implementation is present on the latest Nightly Nightly 65.0a1(2018-11-22)and the latest Beta 64.0b11 as well .
Status: RESOLVED → VERIFIED
status-firefox64: --- → verified
status-firefox65: --- → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: