1435910 - Dragging multiple lines of text to the tab bar creates multiple tabs

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P3)

Description

[ajf]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180201171410

Steps to reproduce:

I'm not sure exactly what I did, but I think some sort of input lag caused me to accidentally select a good portion of TweetDeck and drag it to the tab bar.


Actual results:

Dozens upon dozens of tabs opened containing different “lines” of text from the selection on TweetDeck. If I hadn't manually killed the Firefox content processes, my laptop would be in thrashing hell.


Expected results:

It should have, at worst, created at most one search tab with all the lines concatenated.

Comment 1

[ajf]

Note this requires actual line breaks in the dragged text. This is therefore easier to demonstrate with a textarea (which preserves line breaks when copying/dragging) than with ordinary text on many web pages (which loses them when copying/dragging).

Comment 2

[Cristina Badescu [:cbadescu], Experiments QA]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180206200532

Hello,

I have tested this issue on latest Firefox release 58.0.2, latest Nightly build 60.0a1 (2018-02-09) and managed to reproduce it. I have accessed Tweet Deck application, selected multiple lines of text and dragged them into the tab bar and multiple tabs were opened.

This issue is not reproducible on Chrome and the dragged text is opened in one tab.

I think the suitable component for this issue is Firefox: Tabbed Browser.

Comment 3

[Dão Gottwald [:dao]]

This is intentional as per bug 92737. Gijs, can you confirm?

Comment 4

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #3)
> This is intentional as per bug 92737. Gijs, can you confirm?

I think it is a predictable outcome. I'm not sure I would say it's intentional... In the bug I specifically said:

(In reply to :Gijs from bug 92737 comment #97)
> r- for the duplicate stuff and the breaking change of
> using third party fixup when we didn't use to. AIUI this means if you have
> e.g. "foo bar" as a the 'link' we'll do a web search with the default
> engine. That doesn't seem right. To be clear, you shouldn't need those flags
> for the keyword searches.

I don't recall the context for some of the other things (which was apparently on IRC) - it's been 2 years. :-\

I think doing a single web search would make more sense than doing multiple web searches. Perhaps we should ensure that all items are URIs before making a decision if we get passed a giant pile of newline-separated text.


:arai, do you have cycles to look at this?

Comment 5

[Tooru Fujisawa [:arai]]

Thanks.

I think at least we should warn just like the case middle clicking bookmark folder,
when the dropped text lines (or extracted URLs/search words) are more than 20 or so.

https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/browser/locales/en-US/chrome/browser/places/places.properties#53
> tabs.openWarningMultipleBranded=You are about to open %S tabs.
> This might slow down %S while the pages are loading.
> Are you sure you want to continue?

then, I agree that opening single search for multiple-lines non-URL text is reasonable.
I'll try fix both of them.

Comment 6

[Tooru Fujisawa [:arai]]

I'll post the patch after some test.

Comment 7

[Tooru Fujisawa [:arai]]

the plan is the following:
  * if dropped text has at least one URL, open all URLs, ignoring other texts
  * if dropped text has no URI, open search with the entire text

then the problem is, how to check if the line is "URL"
I was about to filter the string that starts with URI scheme,
but I just noticed that testcases have "mochi.test/first" etc, that is,
text that doesn't have scheme, but starts with hostname.
maybe I should trigger fixup first, and check if there is search case.

Comment 8

[:Gijs (he/him)]

(In reply to Tooru Fujisawa [:arai] from comment #7)
> the plan is the following:
>   * if dropped text has at least one URL, open all URLs, ignoring other texts
>   * if dropped text has no URI, open search with the entire text
> 
> then the problem is, how to check if the line is "URL"
> I was about to filter the string that starts with URI scheme,
> but I just noticed that testcases have "mochi.test/first" etc, that is,
> text that doesn't have scheme, but starts with hostname.
> maybe I should trigger fixup first, and check if there is search case.

That sounds like it should work, yes. If you use the fixup's service getFixupURIInfo then you can both get the URI (if any) and find out if search was used from the resulting info object.

Alternatively, when using fixup for multiple lines, you could just not pass the flag that allows for search fixup. That would restrict the results. On the other hand, that will still fix up e.g. "mozilla" to "http://mozilla/" which probably isn't what you want in that case.

I wish we'd moved our fixup to JS already, and made it less... bizarre. :-(

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 0.1: Move PlacesUIUtils.confirmOpenInTabs into new TabUtils.jsm and add promiseConfirmOpenInTabs.

The confirm dialog for warning when opening too many tabs are implemented in
PlacesUIUtils.confirmOpenInTabs, and it's already used by non-places code in
TabListComponent.js.

Created TabUtils.jsm and moved the method there, and also added async variant,
because it's necessary for drag-and-drop case, since sync dialog cannot be used
in drop event (it throws exception).

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 0.2: Add BrowserTestUtils.{promiseAlertDialogOpen,promiseAlertDialog}.

promiseAlertDialogOpen and its variant is implemented in some places,
and the test for warning when dropping too many item also needs it,
so moved the function to BrowserTestUtils,
and also added BrowserTestUtils.promiseAlertDialog which waits for the dialog
to be closed.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Warn when opening too many tabs by drag and drop.

Now dropping 15 (default) or more links will show warning.
it uses TabUtils.promiseConfirmOpenInTabs because sync dialog cannot be used in
the drop event handler.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Open either URIs or single search when dropping multiple-line text.

Now either all URLs or single search is opened when dropping text.

  * if there's at least one URL, open URLs, ignoring non-URL
  * if there's no URL, open search with the entire text

Comment 13

[:Gijs (he/him)]

Comment on attachment 8951931 [details] [diff] [review]
Part 0.1: Move PlacesUIUtils.confirmOpenInTabs into new TabUtils.jsm and add promiseConfirmOpenInTabs.

Review of attachment 8951931 [details] [diff] [review]:
-----------------------------------------------------------------

I think moving this out of PlacesUIUtils is probably the right call, but this patch has some loose ends that need fixing.

::: browser/components/places/PlacesUIUtils.jsm
@@ -221,5 @@
>    createFixedURI: function PUIU_createFixedURI(aSpec) {
>      return Services.uriFixup.createFixupURI(aSpec, Ci.nsIURIFixup.FIXUP_FLAG_NONE);
>    },
>  
> -  getFormattedString: function PUIU_getFormattedString(key, params) {

This is also called from https://dxr.mozilla.org/mozilla-central/source/browser/components/places/content/browserPlacesViews.js?q=placesuiutils.getformattedstring&redirect_type=single#455 .

::: browser/components/syncedtabs/TabListComponent.js
@@ +11,5 @@
>  
> +XPCOMUtils.defineLazyModuleGetters(this, {
> +  BrowserUITelemetry: "resource:///modules/BrowserUITelemetry.jsm",
> +  TabUtils: "resource://gre/modules/TabUtils.jsm",
> +  Services: "resource://gre/modules/Services.jsm",

If we're altering this anyway, just move Services.jsm to a non-lazy import at the top.

::: toolkit/modules/TabUtils.jsm
@@ +9,5 @@
> +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +ChromeUtils.import("resource://gre/modules/Services.jsm");
> +
> +XPCOMUtils.defineLazyGetter(this, "bundle", function() {
> +  return Services.strings.createBundle("chrome://browser/locale/places/places.properties");

You can't move this to toolkit if we're relying on a browser/ locale file. So I guess these strings would have to move somewhere else, too.

Between this and the Toolkit :: Tabbed Browser component not existing, I wonder if this shouldn't just stay in browser/modules instead. That'd solve a number of issues here.

::: toolkit/modules/moz.build
@@ +142,5 @@
>  with Files('Sqlite.jsm'):
>      BUG_COMPONENT = ('Toolkit', 'Storage')
>  
> +with Files('TabUtils.jsm'):
> +    BUG_COMPONENT = ('Toolkit', 'Tabbed Browser')

This component doesn't exist...

Comment 14

[:Gijs (he/him)]

Comment on attachment 8951932 [details] [diff] [review]
Part 0.2: Add BrowserTestUtils.{promiseAlertDialogOpen,promiseAlertDialog}.

Review of attachment 8951932 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for simplifying this!

::: browser/components/downloads/test/browser/head.js
@@ +173,5 @@
>    });
>  }
>  
>  function promiseAlertDialogOpen(buttonAction) {
> +  return BrowserTestUtils.promiseAlertDialogOpen(buttonAction);

There's only 1 callsite in this directory - please fix up that callsite and remove this level of indirection.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +1597,5 @@
>      }
>      Services.ppmm.removeDelayedProcessScript(kAboutPageRegistrationContentScript);
>    },
>  
> +  async promiseAlertDialogOpen(buttonAction,

Nit: please add jsdoc documentation for this method.

Comment 15

[:Gijs (he/him)]

Comment on attachment 8951933 [details] [diff] [review]
Part 1: Warn when opening too many tabs by drag and drop.

Review of attachment 8951933 [details] [diff] [review]:
-----------------------------------------------------------------

Tentative r=me, see some notes below.

::: browser/base/content/browser.js
@@ +3557,5 @@
>      for (let link of links) {
>        if (link.url) {
>          let data = await getShortcutOrURIAndPostData(link.url);
>          // Allow third-party services to fixup this URL.
>          openNewTabWith(data.url, null, data.postData, aEvent, true);

What does the event get used for here? We don't seem to use it for the new window button...

@@ +6117,5 @@
>        let data = await getShortcutOrURIAndPostData(link.url);
>        urls.push(data.url);
>        postDatas.push(data.postData);
>      }
>      if (lastLocationChange == gBrowser.selectedBrowser.lastLocationChange) {

I assume this prevents any funny business while the dialog is up? Maybe?

::: browser/base/content/tabbrowser.xml
@@ +7686,5 @@
> +            let replace = !!targetTab;
> +            let newIndex = this._getDropIndex(event, true);
> +            let urls = links.map(link => link.url);
> +
> +            let triggeringPrincipal = browserDragAndDrop.getTriggeringPrincipal(event);

I think we should extract this data (and the shift key, `this.selectedItem`, etc. - basically everything above the `loadTabs` call) before opening the dialog. The way this is written right now, some of this data may change or be invalidated if the dialog is up for a while, so we should make sure we get hold of it before it potentially goes away.

::: browser/base/content/test/general/browser_newWindowDrop.js
@@ +131,5 @@
>    let actualWindowOpenCount = 0;
>    let openedWindows = [];
>    let checkCount = function(window) {
> +    if (ignoreFirstWindow) {
> +      ignoreFirstWindow = false;

Can you clarify why we have this change? Should it really be in one of the other csets?

Comment 16

[:Gijs (he/him)]

Comment on attachment 8951934 [details] [diff] [review]
Part 2: Open either URIs or single search when dropping multiple-line text.

Review of attachment 8951934 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thank you!

::: browser/base/content/test/general/browser_newTabDrop.js
@@ +94,5 @@
>  
> +// Open URLs ignoring non-URL.
> +add_task(async function multiple_urls() {
> +  await dropText(`
> +mochi.test/urls0

Is the lack of indent here required? I believe the URI code trims anyway, so you should be able to just add all the indent to make this look more 'natural'. If I'm wrong, just leave it like this, I don't mind all that much...

::: dom/base/contentAreaDropListener.js
@@ +62,5 @@
>        if (types.contains(type)) {
>          data = dt.mozGetDataAt(type, i);
>          if (data) {
>            let lines = data.replace(/^\s+|\s+$/mg, "").split("\n");
> +          if (lines.length === 0) {

Firefox code style is:

if (!lines.length) {

@@ +67,5 @@
> +            return;
> +          }
> +
> +          // For plain text, there are 2 cases:
> +          //   * if there are at least one URI:

Nit: this needs a singular form of the verb, ie "if there is at least one URI"

@@ +71,5 @@
> +          //   * if there are at least one URI:
> +          //       Add all URIs, ignoring non-URI lines, so that all URIs
> +          //       are opened in tabs.
> +          //   * if there's no URI:
> +          //       Add the entire text as single entry, so that the entire

Nit: article before 'single entry', ie "as a single entry"

@@ +80,5 @@
>            for (let line of lines) {
> +            let info = Services.uriFixup.getFixupURIInfo(line, flags);
> +            if (info.fixedURI) {
> +              // Use the original line here, and let the caller decide
> +              // whether perform fixup or not.

Nit: the infinitive verb needs 'to' here: "whether to perform fixup or not"

Comment 17

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to :Gijs from comment #15)
> ::: browser/base/content/browser.js
> @@ +3557,5 @@
> >      for (let link of links) {
> >        if (link.url) {
> >          let data = await getShortcutOrURIAndPostData(link.url);
> >          // Allow third-party services to fixup this URL.
> >          openNewTabWith(data.url, null, data.postData, aEvent, true);
> 
> What does the event get used for here? We don't seem to use it for the new
> window button...

This code is used when the new tab button is placed in the toolbar (not tab bar),
by default, dropping item on it opens the new tab in background,
and if shift is held, the new tab is opened in foreground.

So, I think it's better just passing the extracted data here as well, instead of Event object.
I'll fix it.


> @@ +6117,5 @@
> >        let data = await getShortcutOrURIAndPostData(link.url);
> >        urls.push(data.url);
> >        postDatas.push(data.postData);
> >      }
> >      if (lastLocationChange == gBrowser.selectedBrowser.lastLocationChange) {
> 
> I assume this prevents any funny business while the dialog is up? Maybe?

The code was introduced in bug 846635, especially bug 846635 comment #21.
So, yes, it should work also for dialog's promise.


> ::: browser/base/content/test/general/browser_newWindowDrop.js
> @@ +131,5 @@
> >    let actualWindowOpenCount = 0;
> >    let openedWindows = [];
> >    let checkCount = function(window) {
> > +    if (ignoreFirstWindow) {
> > +      ignoreFirstWindow = false;
> 
> Can you clarify why we have this change? Should it really be in one of the
> other csets?

I added it to ignore domwindowopened for confirm dialog opened by TabUtils.promiseConfirmOpenInTabs.
I'll add comment.

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Part 0.1: Move PlacesUIUtils.confirmOpenInTabs into new TabUtils.jsm and add promiseConfirmOpenInTabs.

* reverted the removal of getFormattedString
 * moved Services.jsm into non-lazy import
 * moved TabUtils.jsm into browser/modules

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Part 0.2: Add BrowserTestUtils.{promiseAlertDialogOpen,promiseAlertDialog}. r=Gijs

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Warn when opening too many tabs by drag and drop. r=Gijs

Comment 21

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Open either URIs or single search when dropping multiple-line text. r=Gijs

Comment 22

[:Gijs (he/him)]

Comment on attachment 8952943 [details] [diff] [review]
Part 0.1: Move PlacesUIUtils.confirmOpenInTabs into new TabUtils.jsm and add promiseConfirmOpenInTabs.

Review of attachment 8952943 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/TabUtils.jsm
@@ +9,5 @@
> +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +ChromeUtils.import("resource://gre/modules/Services.jsm");
> +
> +XPCOMUtils.defineLazyGetter(this, "bundle", function() {
> +  return Services.strings.createBundle("chrome://browser/locale/places/places.properties");

Hm, maybe move the strings somewhere more generic (browser.properties or a tabbrowser file if we have one ?) in a follow-up?

Comment 23

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to :Gijs from comment #22)
> ::: browser/modules/TabUtils.jsm
> @@ +9,5 @@
> > +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> > +ChromeUtils.import("resource://gre/modules/Services.jsm");
> > +
> > +XPCOMUtils.defineLazyGetter(this, "bundle", function() {
> > +  return Services.strings.createBundle("chrome://browser/locale/places/places.properties");
> 
> Hm, maybe move the strings somewhere more generic (browser.properties or a
> tabbrowser file if we have one ?) in a follow-up?

sure.

actually I tried not to touch the string bundle, since, iiuc, moving string bundle item will reset the localization status for the item.
but I agree that moving it to the right place is better.

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 8952943 [details] [diff] [review]
Part 0.1: Move PlacesUIUtils.confirmOpenInTabs into new TabUtils.jsm and add promiseConfirmOpenInTabs.

Can you please document the purpose of TabUtils.jsm and/or give it a clearer name? I'm concerned this could become yet another dumping ground for entirely unrelated stuff. I'm also curious why you added a jsm at all rather than implementing this in tabbrowser like warnAboutClosingTabs.

Comment 25

[Tooru Fujisawa [:arai]]

(In reply to Dão Gottwald [::dao] from comment #24)
> Comment on attachment 8952943 [details] [diff] [review]
> Part 0.1: Move PlacesUIUtils.confirmOpenInTabs into new TabUtils.jsm and add
> promiseConfirmOpenInTabs.
> 
> Can you please document the purpose of TabUtils.jsm and/or give it a clearer
> name? I'm concerned this could become yet another dumping ground for
> entirely unrelated stuff. I'm also curious why you added a jsm at all rather
> than implementing this in tabbrowser like warnAboutClosingTabs.

Sounds reasonable.
I'll try moving it to tabbrowser.

I've added it because the original implementation was in other jsm (PlacesUIUtils.jsm),
so I thought just moving it to more generic jsm is better.

Comment 26

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #25)
> (In reply to Dão Gottwald [::dao] from comment #24)
> > Comment on attachment 8952943 [details] [diff] [review]
> > Part 0.1: Move PlacesUIUtils.confirmOpenInTabs into new TabUtils.jsm and add
> > promiseConfirmOpenInTabs.
> > 
> > Can you please document the purpose of TabUtils.jsm and/or give it a clearer
> > name? I'm concerned this could become yet another dumping ground for
> > entirely unrelated stuff. I'm also curious why you added a jsm at all rather
> > than implementing this in tabbrowser like warnAboutClosingTabs.
> 
> Sounds reasonable.
> I'll try moving it to tabbrowser.

it turns out that it cannot be a tabbrowser property, since PlacesUIUtils needs to show the confirm on the library window if the open command is triggered from the library window, and there may be no navigator window opened yet, at the point of showing confirm.

I'll rename the jsm and add description.

Comment 27

[Tooru Fujisawa [:arai]]

Attachment: Part 0.1: Move PlacesUIUtils.confirmOpenInTabs into new OpenInTabsUtils.jsm and add promiseConfirmOpenInTabs. r=Gijs

Renamed it to OpenInTabsUtils.jsm and added description,
and also moved strings into tabbrowser.properties

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 8953314 [details] [diff] [review]
Part 0.1: Move PlacesUIUtils.confirmOpenInTabs into new OpenInTabsUtils.jsm and add promiseConfirmOpenInTabs. r=Gijs

>+  confirmOpenInTabs(numTabsToOpen, aWindow) {
>+    const WARN_ON_OPEN_PREF = "browser.tabs.warnOnOpen";
>+    var reallyOpen = true;
>+
>+    if (Services.prefs.getBoolPref(WARN_ON_OPEN_PREF)) {
>+      if (numTabsToOpen >= Services.prefs.getIntPref("browser.tabs.maxOpenBeforeWarn")) {
>+        // default to true: if it were false, we wouldn't get this far
>+        var warnOnOpen = { value: true };
>+
>+        var messageKey = "tabs.openWarningMultipleBranded";
>+        var openKey = "tabs.openButtonMultiple";
>+        const BRANDING_BUNDLE_URI = "chrome://branding/locale/brand.properties";
>+        var brandShortName = Services.strings.
>+                             createBundle(BRANDING_BUNDLE_URI).
>+                             GetStringFromName("brandShortName");
>+
>+        var buttonPressed = Services.prompt.confirmEx(
>+          aWindow,
>+          this.getString("tabs.openWarningTitle"),
>+          this.getFormattedString(messageKey, [numTabsToOpen, brandShortName]),
>+          (Services.prompt.BUTTON_TITLE_IS_STRING * Services.prompt.BUTTON_POS_0) +
>+            (Services.prompt.BUTTON_TITLE_CANCEL * Services.prompt.BUTTON_POS_1),
>+          this.getString(openKey), null, null,
>+          this.getFormattedString("tabs.openWarningPromptMeBranded",
>+                                  [brandShortName]),
>+          warnOnOpen
>+        );

nit: replace var with let while you're moving this code

Comment 29

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e09c5d2d68487516af11877d9c69ced4f8266d0
Bug 1435910 - Part 0.1: Move PlacesUIUtils.confirmOpenInTabs into new OpenInTabsUtils.jsm and add promiseConfirmOpenInTabs. r=Gijs,dao

https://hg.mozilla.org/integration/mozilla-inbound/rev/6331c25d7425cc1f1fdf72aa96376d26e503e3e5
Bug 1435910 - Part 0.2: Add BrowserTestUtils.{promiseAlertDialogOpen,promiseAlertDialog}. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e8d6f7246d15e550ec8448b1895cb7dbfba8ed
Bug 1435910 - Part 1: Warn when opening too many tabs by drag and drop. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3ff1926cc93e0300771d215bc037906d291570b
Bug 1435910 - Part 2: Open either URIs or single search when dropping multiple-line text. r=Gijs

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4e09c5d2d684
https://hg.mozilla.org/mozilla-central/rev/6331c25d7425
https://hg.mozilla.org/mozilla-central/rev/f6e8d6f7246d
https://hg.mozilla.org/mozilla-central/rev/f3ff1926cc93

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Too late for 59.