Closed Bug 1324410 Opened 7 years ago Closed 7 years ago

cannot drop selected text on Location Bar from content area

Categories

(Firefox :: Address Bar, defect)

53 Branch
x86
Windows 10
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 + verified

People

(Reporter: alice0775, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(1 file)

[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
Flags: needinfo?(gijskruitbosch+bugs)
Isn't it wonderful we have all these features for which we have no automated tests. :-(

Thanks for finding and filing this, Alice.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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.
(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.
(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...
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.)
Tracking 53+ for this drag and drop regression, which is a common operation.
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 on attachment 8819985 [details]
Bug 1324410 - fix dnd to the location bar for plaintext,

https://reviewboard.mozilla.org/r/99546/#review101014
Attachment #8819985 - Flags: review?(mak77)
Flags: needinfo?(gijskruitbosch+bugs)
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.
Flags: needinfo?(gijskruitbosch+bugs)
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
Attachment #8819985 - Flags: review?(mak77) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7261ee7e3e67
fix dnd to the location bar for plaintext, r=mak
https://hg.mozilla.org/mozilla-central/rev/7261ee7e3e67
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.