1435562 - `browser.tabs.warnOnOpen` is not respected when opening multiple items from Library

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Bruce]

Steps to reproduce:

1. Make sure that `browser.tabs.warnOnOpen` is at its default value of `true`.
2. Open the Library window. Switch to the history for a particular day. (Can also open a bookmark folder or a particular tag - basically anything that puts a bunch of non-container items in the panel on the right.)
3. From the rhs panel, select >=15 items (=`browser.tabs.maxOpenBeforeWarn`). Right click > Open All in Tabs.

Actual behaviour:

Firefox opens all the tabs without warning.

Expected behaviour:

Firefox should have warned.

Comment 1

[Mihir Karbelkar(:fauxwizard_)]

I would like to work on this. Thanks.

Comment 2

[Mark Banner (:standard8)]

Hi Mihir, thank you for taking this, please work on it and when you attach a patch, we'll assign it to you.

For implementing this, I'd suggest taking a look at the existing behaviour in the library - when right-clicking folders and selecting "Open All in Tabs" we do the right thing and prompt, but we don't do that when we're in the left-hand pane.

We probably need to unify the two routes somehow.

If you have any questions, please feel free to ask here or on irc.

Comment 3

[Toby Ward]

Hi,

Just so that everyone knows, I'm currently working on this bug at the moment. I've got a fix written already and am currently writing a unit test to prevent regressions in the future. I'm currently planning on submitting a diff once this test has been written, although I'm also happy to separate the fix and test if desired.

Toby

Comment 4

[Toby Ward]

Attachment: browser_library_warnOnOpen.js

Hi,

I've made good progress with the bug so far, and currently have both a fix and one test working so far (opening a large folder). However, I'm having a bit of trouble working out how to open the context menu after selecting multiple links;

The test for opening when selecting multiple links is between L85-128. I can successfully populate the bookmarks with my examples and select them all (this was tested by using a test from browser_paste_bookmarks.js [1]). However, I don't really have a good enough understanding on how to use synthesizeMouseAtCenter to open up the context menu like in [2]; I've currently got it set up to use targetBookmarks[0] as aTarget, but I've also tried using getToolbarNodeForItemGuid(targetBookmarks[0].guid) without any luck either as it returns null.

[1] https://searchfox.org/mozilla-central/source/browser/components/places/tests/browser/browser_paste_bookmarks.js#95
[2] https://searchfox.org/mozilla-central/source/browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js#78

Comment 5

[Mark Banner (:standard8)]

I think the best thing to do here is to use this:

```
  synthesizeClickOnSelectedTreeCell(gLibrary.ContentTree.view, {
    button: 2,
    type: "contextmenu",
  });
```

This is a function that's already in browser/components/places/tests/browser/head.js. The only slight issue is that you'll need to remove the check for only one item in the selection - that check isn't really necessary as the function still works fine for multiple selections.

Note, a few lines above the failure point you're getting the context menu and openTabs, however currently you're getting them from the main window (where the scope of the test is), rather than the library window, so you'll need to prefix the `document.getElement....` with `gLibrary.`.

Comment 6

[Toby Ward]

Attachment: Bug 1435562 - browser.tabs.warnOnOpen is now respected when opening multiple items from the library. r=Standard8

- Unified openContainerNodeInTabs and openURINodesInTabs in PlacesUIUtils into openMultipleLinksInTabs
- Users are now warned when the amount of links to be opened is equal to or exceeds browser.tabs.maxOpenBeforeWarn

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81a6b32f2a8b
browser.tabs.warnOnOpen is now respected when opening multiple items from the library. r=Standard8

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/81a6b32f2a8b

Comment 9

[Natalia Csoregi [:nataliaCs]]

Backed out for causing Bug 1512180

backout: https://hg.mozilla.org/mozilla-central/rev/3f89cd6708a815c585181d38c67d0eea5bdc9090

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a95415be71a3
browser.tabs.warnOnOpen is now respected when opening multiple items from the library. r=Standard8

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/a95415be71a3

Comment 12

[Sajedul Islam]

I have reproduced this bug with Nightly 60.0a1 (2018-02-03) on Windows 10, 64 Bit!

This bug's fix is verified with latest Nightly!

Build ID : 20190128092811
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0

Comment 13

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Thank you Sajedul for verifying this!