Closed Bug 1402380 Opened 2 years ago Closed 2 years ago

Unexpected Google search is performed with garbage words when drag & drop a bookmarks folder from Library window to tab bar

Categories

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

52 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: alice0775, Assigned: arai)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

Reproducible: always

Steps To Reproduce:
1. drag & drop a bookmarks folder from Library window to tab bar

Actual Results:
Unexpected Google search is performed with garbage words (i.e folder name and containing urls)

Expected Results:
a) Nothing happens (the behavior of Firefox 51).
or
b) Open all bookmarks in new tabs (the behavior of "Open All in Tabs")


Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f22edbb9d56c6e5aee8426be0b706bbbfe43f604&tochange=d87a29e5c993089a50126693d9b71773917c33ac

Regressed by: Bug 92737
Component: Bookmarks & History → Drag and Drop
Product: Firefox → Core
Thanks.

the reasons of this issue are following:
  * the data transfer contains text/x-moz-url flavor but it's empty
    this flavor is ignored and fallbacks to other way
  * the data transfer contains text/plain flavor with the following content
    (the text corresponds to the HTML representation of the folder)
    * dragged folder's name
    * URL for each bookmark item, including items inside inner folders
    * "--------------------" for each separator
    * folder name for each folder inside it
    this flavor is used, and URLs are opened and non-URL items are searched or forcibly opened as URL

https://dxr.mozilla.org/mozilla-central/rev/2cd3752963fc8f24f7c202687eab55e83222f608/dom/base/contentAreaDropListener.js#51-74

maybe we could set text/x-moz-url with URLs for items directly under the folder, excluding items inside inner folders
since currently middle-clicking the folder opens only them.
and the place that gathers URLs for middle-click is here
https://dxr.mozilla.org/mozilla-central/rev/2cd3752963fc8f24f7c202687eab55e83222f608/toolkit/components/places/PlacesUtils.jsm#1417-1446

we could extend that method to gather item name too, and serialize it as text/x-moz-url in caller.
this at least fixes the issue.
I'll test other cases and add testcase later.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
arai, are you still thinking about working on this? Looks like you have a WIP patch.
oh, I forgot about this.
I'll look into it this weekend.
I guess, it's not possible to test the behavior for dragstart handler properly?
so far this code fixes the issue, but there's no automated test.
Attachment #8911332 - Attachment is obsolete: true
Flags: needinfo?(arai.unmht)
Attachment #8957085 - Flags: review?(mak77)
Attachment #8957085 - Flags: review?(mak77) → review?(standard8)
Can someone clarify the steps to repeat here please?

I'm on latest nightly on Mac, and I've just been dragging a folder from the library window to the tab bar, and everything is opening correctly. There's no search performed.

If I drag to the location bar, then just the first page gets opened (which I'd expect as it is a text box).
Flags: needinfo?(arai.unmht)
Here's steps:
  1. run Nightly 61.0a1 (2018-03-12) (64-bit) with clean profile on macOS
  2. show Bookmark Toolbar
  3. create a New Folder in Bookmark Toolbar, with name "a"
  4. create the following bookmarks inside folder "a":
     * separator
     * https://bugzilla.mozilla.org/
  5. drag the folder "a" into the next to current tab (empty area)

Actual result:
  the following tabs are opened:
    * search for "a"
    * search for "--------------------"
    * https://bugzilla.mozilla.org/

Expected result:
  * only https://bugzilla.mozilla.org/ is opened
Flags: needinfo?(arai.unmht)
Thank you, a little bit of additional clarification: the search for the folder name doesn't happen if there's a space in it.
yes, that’s what fixed by bug 1435910.
if the text looks like domain name or URL, it doesn’t work well.

anyway, setting URL list fixes the issue here
Comment on attachment 8957085 [details] [diff] [review]
Set text/x-moz-url flavor of data transfer for places container with URLs inside it.

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

I think the general idea here is good, but we need tests to land this.

I would suggest looking at browser_toolbar_drop_text.js to get inspiration for a test. Or any test that uses EventUtils.synthesizeDrop.

If you need help feel free to ask/ping me.

https://searchfox.org/mozilla-central/source/browser/components/places/tests/browser/browser_toolbar_drop_text.js
Attachment #8957085 - Flags: review?(standard8) → feedback+
Thank you for your feedback :)

(In reply to Mark Banner (:standard8) (afk until 19 Mar) from comment #13)
> Comment on attachment 8957085 [details] [diff] [review]
> Set text/x-moz-url flavor of data transfer for places container with URLs
> inside it.
> 
> Review of attachment 8957085 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the general idea here is good, but we need tests to land this.
> 
> I would suggest looking at browser_toolbar_drop_text.js to get inspiration
> for a test. Or any test that uses EventUtils.synthesizeDrop.
> 
> If you need help feel free to ask/ping me.
> 
> https://searchfox.org/mozilla-central/source/browser/components/places/tests/
> browser/browser_toolbar_drop_text.js

synthesizeDrop sets dataTransfer on test's side, but what we need to test is that part in browser's side, to see the expected data is set to dataTransfer when user starts dragging the folder item.
so, I think there's no existing way to check it.
I'll try implementing another helper function, to see if it's possible in automation.
Maybe you could manually involve the dragstart handler to get the transfer data:

https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/browser/components/places/content/tree.xml#742

That would work from the library or sidebar at least, though there should be one for the toolbar as well.
(In reply to Mark Banner (:standard8) (afk until 19 Mar) from comment #16)
> Maybe you could manually involve the dragstart handler to get the transfer
> data:
> 
> https://searchfox.org/mozilla-central/rev/
> 8976abf9cab8eb4661665cc86bd355cd08238011/browser/components/places/content/
> tree.xml#742
> 
> That would work from the library or sidebar at least, though there should be
> one for the toolbar as well.

thanks, looks like the above try is fine, except that I forgot to call endDragSession, that results in the failure in the next test.
I'll post the patch after extra try.
Added EventUtils.synthesizePlainDragAndDrop that try to emulate plain drag and drop, without modifying anything (compared to EventUtils.synthesizeDrop).
and added testcase that uses it, and checks if expected tabs are opened and no other tabs are opened.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84708f35de519517bf84cf59500d5b1b24eee3a6
Attachment #8957085 - Attachment is obsolete: true
Attachment #8959721 - Flags: review?(standard8)
Some clarification about synthesizePlainDragAndDrop and synthesizeDrop.

I've added synthesizePlainDragAndDrop because the behavior of synthesizeDrop is really different than the case that user initiates the drag and drop.
especially:
  * synthesizeDrop modifies dataTransfer, that won't happen on the actual case
  * synthesizeDrop dispatches events within the same event tick, that won't happen on the actual case
    also if those events are dispatched in the same event tick, UI things doesn't respond to them in the expected way
    in this bug's case, Menu is not yet opened after mousedown, before mousemove
At first, I tried to modify synthesizeDrop to receive extra parameter to change the behavior, but it makes things really complicated (and there are so many parameters),
so I added dedicated method that does simple plain drag and drop, without anything non-default, also with an object parameter to clarify which parameter is what.

maybe we should merge them ultimately, by deprecating synthesizeDrop and moving non-default behavior into synthesizePlainDragAndDrop (and rename it), but I'd like to leave it to other bug.
Comment on attachment 8959721 [details] [diff] [review]
Set text/x-moz-url flavor of data transfer for places container with URLs inside it.

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

Neil, the EventUtils.js addition looks reasonable to me, please could you take a look at it as you know more about drag and drop?

Tooru, thanks for the test addition, it looks ok so far, though I think we can use some more of the helper methods and get a cleaner test. Please request review from myself (and Neil if necessary) when you update it.

::: browser/components/places/tests/browser/browser_drag_folder_on_newTab.js
@@ +26,5 @@
> +});
> +
> +const TEST_FOLDER_NAME = "Test folder";
> +
> +async function CreateTestFolder() {

nit: small 'c' at the start of the function name please.

@@ +27,5 @@
> +
> +const TEST_FOLDER_NAME = "Test folder";
> +
> +async function CreateTestFolder() {
> +  let folder = await PlacesUtils.bookmarks.insert({

Please use insertTree rather than insert. It is quicker & simpler to understand. We can do this in-line in the setup function as well.

@@ +62,5 @@
> +
> +  await CreateTestFolder();
> +
> +  let toolbar = document.getElementById("PersonalToolbar");
> +  let folder = Array.from(toolbar.getElementsByClassName("bookmark-item"))

If you save the created folder guid, you can use getToolbarNodeForItemGuid to get the node.

@@ +79,5 @@
> +        resolve();
> +      }
> +    };
> +  });
> +  gBrowser.tabContainer.addEventListener("TabOpen", handler);

Please use BrowserTestUtils.waitForNewTab, it is more reliable than spinning our own handlers and we can easily match the urls that are opened. Here's an example:

https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/browser/components/places/tests/browser/browser_sidebar_open_bookmarks.js#53-58,62

@@ +107,5 @@
> +  }
> +
> +  gBrowser.tabContainer.removeEventListener("TabOpen", handler);
> +  is(actualTabOpenCount, expectedTabOpenCount,
> +     "No other tabs are opened");

This check isn't necessary, as the test harness does that for us.

::: testing/mochitest/tests/SimpleTest/EventUtils.js
@@ +2607,5 @@
>  
> +/**
> + * Emulate a drag and drop by emulating a dragstart and firing events dragenter,
> + * dragover, and drop.
> + * This does not modify dataTransfer and try to emulate the plain drag and drop

nit: s/try/tries/
Attachment #8959721 - Flags: review?(standard8) → review?(enndeakin)
Although this was a regression from a core bug, we're handling it in bookmarks/places, so we should move it across to a relevant component.
Component: Drag and Drop → Bookmarks & History
Priority: -- → P2
Product: Core → Firefox
Thank you for reviewing :D

(In reply to Mark Banner (:standard8) from comment #20)
> @@ +79,5 @@
> > +        resolve();
> > +      }
> > +    };
> > +  });
> > +  gBrowser.tabContainer.addEventListener("TabOpen", handler);
> 
> Please use BrowserTestUtils.waitForNewTab, it is more reliable than spinning
> our own handlers and we can easily match the urls that are opened. Here's an
> example:

it's using raw addEventListener to catch unexpected tabs, see below.


> https://searchfox.org/mozilla-central/rev/
> 877c99c523a054419ec964d4dfb3f0cadac9d497/browser/components/places/tests/
> browser/browser_sidebar_open_bookmarks.js#53-58,62
> 
> @@ +107,5 @@
> > +  }
> > +
> > +  gBrowser.tabContainer.removeEventListener("TabOpen", handler);
> > +  is(actualTabOpenCount, expectedTabOpenCount,
> > +     "No other tabs are opened");
> 
> This check isn't necessary, as the test harness does that for us.

Yeah, I thought so, but bug 1442167 was caused by opening more than expected tabs, and overlooked because test harness doesn't report it.
maybe we should check test harness.
(In reply to Tooru Fujisawa [:arai] from comment #22)
> > @@ +107,5 @@
> > > +  }
> > > +
> > > +  gBrowser.tabContainer.removeEventListener("TabOpen", handler);
> > > +  is(actualTabOpenCount, expectedTabOpenCount,
> > > +     "No other tabs are opened");
> > 
> > This check isn't necessary, as the test harness does that for us.
> 
> Yeah, I thought so, but bug 1442167 was caused by opening more than expected
> tabs, and overlooked because test harness doesn't report it.
> maybe we should check test harness.

As I've just commented there, I think the analysis of the failure mode was not quite right. They're a different style of test that failed, so easy to miss. Hence I'm wary to go that route. waitForNewTab provides us with a more deterministic way of waiting for the tabs, and we can check that the expected URLs have been opened (e.g. we could be opening a number of tabs all with the same url and still pass this test at the moment).

The other issue about attempting to check for extra tabs being opened, is that you can't actually be sure when they have finished opening, meaning you have to insert timeouts or extra waits which leads to slower tests / potential random issues. That shouldn't be necessary, and I know from recent experience that tests do tell you when tabs are left open, so in summary, I think we're safe to go with waitForNewTab.
Comment on attachment 8959721 [details] [diff] [review]
Set text/x-moz-url flavor of data transfer for places container with URLs inside it.

The EventUtils.js changes look ok, but you might want to clarify in the documentation which events are emulated. The comment mentions: dragstart, dragenter, dragover and drop, but not the other drag events.

> + *          destElement:  The element to drop

'drop on'
Attachment #8959721 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4cc022b417a1268fa063fe054aa79f3c141c2e2
Bug 1402380 - Set text/x-moz-url flavor of data transfer for places container with URLs inside it. r=enndeakin
https://hg.mozilla.org/mozilla-central/rev/e4cc022b417a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Please request Beta approval on this when you get a chance.
Flags: needinfo?(arai.unmht)
Flags: in-testsuite+
here's updated patch that have landed to m-c,
which is also applicable to mozilla-beta.


Approval Request Comment
> [Feature/Bug causing the regression]
bug 92737

> [User impact if declined]
when an user drops bookmark folder onto tab bar, some unexpected search is performed with folder names and separator text.

> [Is this code covered by automated tests?]
Yes

> [Has the fix been verified in Nightly?]
Yes

> [Needs manual test from QE? If yes, steps to reproduce]
No

> [List of other uplifts needed for the feature/fix]
None

> [Is the change risky?]
No

> [Why is the change risky/not risky?]
This patch adds better information to drag-and-drop data transfer for bookmark folder.
No underlying behavior is changed but the drop handler just uses the added information.

> [String changes made/needed]
None
Attachment #8959721 - Attachment is obsolete: true
Flags: needinfo?(arai.unmht)
Attachment #8962558 - Flags: review+
Attachment #8962558 - Flags: approval-mozilla-beta?
Comment on attachment 8962558 [details] [diff] [review]
Set text/x-moz-url flavor of data transfer for places container with URLs inside it. r=enndeakin

feels borderline for uplift (old bug, corner case), but I guess we can take this in beta60 still.
Attachment #8962558 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.