1820332 - Dragging URL from address bar results in bookmark with the URL instead of the page title

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Tom S [:evilpie]]

STR:

• Go to example.org
• Highlight the text "example.org" in the address bar and start dragging
• Drop the dragged URL onto the bookmark bar

Expected behavior:
A bookmark with the name "Example Domain" is created.

Actual Behavior:
A bookmark with name "http://example.org" is created.

mozregression points to bug 1776879 as the most likely regressor. This was reported on reddit.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1776879

:enndeakin, since you are the author of the regressor, bug 1776879, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 2

[Tom S [:evilpie]]

The problem seems to be with PlacesControllerDragHelper.onDrop and the call to getFirstValidFlavor. That always always picks text/plain instead of text/x-moz-url. The former doesn't contain the title of the dragged link.

This change worked for me:

    for (const supported of PlacesUIUtils.SUPPORTED_FLAVORS) {
      for (let i = 0; i < aFlavors.length; i++) {
        if (aFlavors[i] == supported) {
          return supported;
        }
      }
    }

Comment 3

[Marco Bonardo [:mak]]

(In reply to Tom S [:evilpie] from comment #2)

The problem seems to be with PlacesControllerDragHelper.onDrop and the call to getFirstValidFlavor. That always always picks text/plain instead of text/x-moz-url. The former doesn't contain the title of the dragged link.

I'd probably just do

return PlacesUIUtils.SUPPORTED_FLAVORS.find(f => aFlavors.includes(f))

And then it will need a test.

Though I wonder why the order of flavors changed, the caller should always put their preferred flavor at the top, or not?

Comment 4

[Tom S [:evilpie]]

aFlavors is just the DOMStringList returned by the call to mozTypesAt, so the order is just probably not strictly defined. I am not sure why the order changed.

Comment 5

[Marco Bonardo [:mak]]

It used to not be "random", indeed I remember, back in the days, recommendation to populate the flavors list in a specific order of preference.

Comment 6

[Neil Deakin]

When dragging the selected text, the data being dragged is in the DataTransfer in this order:
text/plain
text/x-moz-url
text/html

It seems like it should be added with text/x-moz-url first since it is more specific. But the code at https://searchfox.org/mozilla-central/rev/00ea1649b59d5f427979e2d6ba42be96f62d6e82/browser/components/urlbar/UrlbarInput.sys.mjs#3617. which adds the data appears to put urls in before plaintext. It's possible the spec version of DataTransfer doesn't preserve the order, I haven't checked, but the original implementation does do this and the documentation says this.

However, my testing on an older build suggests that the resulting order doesn't appear to have changed since 1776879, so there is a second issue here.

Comment 7

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

:mak, any idea on the severity here? do we want to try to get a fix in for this regression for 111?

Comment 8

[Neil Deakin]

I investigated this more and the old code had SUPPORTED_FLAVORS with text/unicode in it, but used it to look up in a DataTransfer which only uses text/plain. The result is that the text version would never be found, and the url type would be used instead.

The new code uses text/plain exclusively so finds the plain text instead.

For some reason the DataTransfer isn't returning the types in the same order they were added. I'll investigate that further. The places code is also iterating over the types in the order they appear in the DataTransfer, but it can and should iterate over the SUPPORTED_FLAVORS order instead. This is what is suggested in comment 3, so I think this is the change that should be made.

Comment 9

[Marco Bonardo [:mak]]

(In reply to Jared Hirsch [:jhirsch] (he/him) (Needinfo please) from comment #7)

:mak, any idea on the severity here? do we want to try to get a fix in for this regression for 111?

This is a papercut kind of bug, if you do this operation often it will be annoying, but there's various workarounds, like dragging the tab, or the lock, or even just editing the bookmark. Thus I think it's an S3.
We should have a fix in 112. 111 is a nice-to-have but I don't think it's super critical if we miss it.

Comment 10

[Marco Bonardo [:mak]]

Should we keep this bug to investigate the DataTransfer not preserving order issue, and change Places code in a separate bug?

Comment 11

[Neil Deakin]

I'd suggest we just fix the places code in this bug.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1776879

Comment 14

[Marco Bonardo [:mak]]

Attachment: Bug 1820332 - Dragging URL from address bar results in bookmark with the URL instead of the page title. r=neildeakin

Comment 15

[Neil Deakin]

I filed bug 1824740 on the issue with DataTransfer.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/813e6593fe55
Dragging URL from address bar results in bookmark with the URL instead of the page title. r=NeilDeakin

Comment 17

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/813e6593fe55

Comment 18

[Ardelean Oana, Desktop QA [:oardelean]]

Reproducible on a 2023-03-25 Nightly build on macOS 12.
Verified as fixed on Firefox 113.0b5(build ID: 20230418175842) and Nightly 114.0a1(build ID: 20230419214510) on macOS 12, Windows 10, Ubuntu 22.