1324410 - cannot drop selected text on Location Bar from content area

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: Cannot drag&drop selected text on Locationbar

Steps to reproduce:
1. select text in content area
2. drag and drop the selected text on Location Bar

Actual Results:
Cannot drop

Expected Results:
Can drop.
Selected text should appear in the Location bar

Comment 1

[:Gijs (out for now; he/him)]

Isn't it wonderful we have all these features for which we have no automated tests. :-(

Thanks for finding and filing this, Alice.

Comment 2

[Marco Bonardo [:mak]]

indeed we disallow dropping non-links, while we should disallow dropping certain links :(
I don't think it's a surprise that this is untested, it's very basic d&d ops, it's hard to have full coverage of such things.

Comment 3

[avada]

(In reply to :Gijs Kruitbosch from comment #1)
> Isn't it wonderful we have all these features for which we have no automated
> tests. :-(
> 
> Thanks for finding and filing this, Alice.

Hi!

I actually found it, and asked others on mozillazine.
Anyway. When no-one responded I filed a ~identical bug myself just minutes before this: Bug 1324409


So, please dupe one of them.

Comment 4

[:Gijs (out for now; he/him)]

(In reply to Marco Bonardo [::mak] from comment #2)
> indeed we disallow dropping non-links, while we should disallow dropping
> certain links :(
> I don't think it's a surprise that this is untested, it's very basic d&d
> ops, it's hard to have full coverage of such things.

We've never done something to specifically 'allow' dropping text - but it was in the list of types for which we preventDefault() on dragover, which it now isn't, so I suspect that's it...

Comment 6

[:Gijs (out for now; he/him)]

(In reply to avada from comment #3)
> (In reply to :Gijs Kruitbosch from comment #1)
> > Isn't it wonderful we have all these features for which we have no automated
> > tests. :-(
> > 
> > Thanks for finding and filing this, Alice.
> 
> Hi!
> 
> I actually found it, and asked others on mozillazine.

Thread link? I don't normally read mozillazine.

> Anyway. When no-one responded I filed a ~identical bug myself just minutes
> before this: Bug 1324409

Wow, subsequent bug numbers.

There are 2 important distinctions here that explain the difference in why this bug got attention and yours didn't.

1) Alice found/listed the regressing bug and marked it blocking the bug they filed
2) Alice needinfo'd the person who regressed it (ie me) based on the regressing bug.

(and potentially, if that hadn't been enough, there's the tracking flag to request tracking for regression bugs that are new in a particular release)

All of those are useful ways of ensuring the right people see bugs you file. Of course, they don't always all apply. In general, Marco triages bugs in the Location Bar component (and I triage bugs in some other components) so someone will eventually look at bugs you file in watched components anyway, but it will take longer without a needinfo and/or it being a regression.

> So, please dupe one of them.

Done.

Comment 7

[avada]

(In reply to :Gijs Kruitbosch from comment #6)
> 
> Thread link? I don't normally read mozillazine.
> 

I don't have further details, other than what I added to the dupe.

> There are 2 important distinctions here that explain the difference in why
> this bug got attention and yours didn't.
> 
> 1) Alice found/listed the regressing bug and marked it blocking the bug they
> filed
> 2) Alice needinfo'd the person who regressed it (ie me) based on the
> regressing bug.
> 
> (and potentially, if that hadn't been enough, there's the tracking flag to
> request tracking for regression bugs that are new in a particular release)
> 

Okay. Though I didn't know the regressing bug, so I couldn't do any of those things. (Except the tracking flag, which I didn't think of.)

Comment 8

[Marcia Knous [:marcia]]

Tracking 53+ for this drag and drop regression, which is a common operation.

Comment 9

[:Gijs (out for now; he/him)]

Attachment: Bug 1324410 - fix dnd to the location bar for plaintext,

Review commit: https://reviewboard.mozilla.org/r/99546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99546/

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8819985 [details]
Bug 1324410 - fix dnd to the location bar for plaintext,

https://reviewboard.mozilla.org/r/99546/#review100620

::: browser/base/content/urlbarBindings.xml:724
(Diff revision 1)
>              let strippedURL = stripUnsafeProtocolOnPaste(url);
>              if (strippedURL != url) {
>                aEvent.stopImmediatePropagation();
> +              aEvent.dataTransfer.dropEffect = "none";
>                return null;
>              }

I'm not totally convinced of the code layout...
I'd make _getDroppableItem return null for unsafe link, url or the original text otherwise.

in onDragOver I'd keep it like
if (!this._getDroppableItem(aEvent)) {
  aEvent.dataTransfer.dropEffect = "none";
}

in onDrop I'd do

let droppedItem = this._getDroppableItem(aEvent);
...all the remaining code that is currently under (!aIsDragOver) and finally the current onDrop code.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8819985 [details]
Bug 1324410 - fix dnd to the location bar for plaintext,

https://reviewboard.mozilla.org/r/99546/#review101014

Comment 12

[:Gijs (out for now; he/him)]

Don't request needinfo without asking any questions. I'm aware of this bug and will return to it as soon as I can. It's tracking 53. We won't ship it.

Comment 13

[:Gijs (out for now; he/him)]

Comment on attachment 8819985 [details]
Bug 1324410 - fix dnd to the location bar for plaintext,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99546/diff/1-2/

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8819985 [details]
Bug 1324410 - fix dnd to the location bar for plaintext,

https://reviewboard.mozilla.org/r/99546/#review106306

::: browser/base/content/urlbarBindings.xml:706
(Diff revision 2)
>          ]]></body>
>        </method>
>  
> -      <method name="_getDroppableLink">
> +      <!-- Returns:
> +           null if there's a security issue and we should do nothing.
> +           a URL object if there is one that we're OK with loading

nit: comma after loading

Comment 15

[:Gijs (out for now; he/him)]

Comment on attachment 8819985 [details]
Bug 1324410 - fix dnd to the location bar for plaintext,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99546/diff/2-3/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7261ee7e3e67
fix dnd to the location bar for plaintext, r=mak

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7261ee7e3e67

Comment 18

[Azmina]

I have reproduced this bug on Nightly 53.0a1 (2016-12-19), Windows 8.1(64-bit).

This bug's fix is verified on Latest Nightly 53.0a1.

Build ID : 20170119030211
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0

[bugday-20170118]