Last Comment Bug 717772 - Delay autocomplete of pasted value
: Delay autocomplete of pasted value
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: All All
-- normal (vote)
: Firefox 12
Assigned To: Marco Bonardo [::mak]
: Marco Bonardo [::mak]
Depends on: 778391
Blocks: 566489 727954
  Show dependency treegraph
Reported: 2012-01-12 14:53 PST by Marco Bonardo [::mak]
Modified: 2014-06-08 11:35 PDT (History)
9 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

hack (2.39 KB, patch)
2012-01-13 08:31 PST, Marco Bonardo [::mak] feedback+
Details | Diff | Splinter Review
wip (7.23 KB, patch)
2012-01-16 08:12 PST, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.0 (13.56 KB, patch)
2012-01-17 06:08 PST, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v2.0 (10.13 KB, patch)
2012-01-18 07:46 PST, Marco Bonardo [::mak] review+
Details | Diff | Splinter Review

Description User image Marco Bonardo [::mak] 2012-01-12 14:53:34 PST
There are security concerns in allowing to inline autocomplete pasted partial urls, since a user may paste a partial url and click enter without noticing the url he is going to is actually different from what he expected.
Comment 1 User image Marco Bonardo [::mak] 2012-01-12 15:38:32 PST
we may need a custom cmd_paste controller to detect this, similar to the _copyCutController one... I don't have better ideas atm, so any alternative idea is welcome.
Comment 2 User image Marco Bonardo [::mak] 2012-01-13 08:31:14 PST
Created attachment 588430 [details] [diff] [review]

I don't have better ideas atm, this works as far as the autocomplete fills up synchronously, that is not always true (if we fail matching on a domain it's likely a subpage and autofill for that happens asynchronously). So it's not yet the solution, it's a possible hook point though, that I would like some feedback or ideas on. Maybe we just care about not autocompleting domain matches and not subpages? in such a case it would be enough.
Otherwise we could maybe store the last pasted value and compare with it in the first textValue setter call. It's still sorta hackish though.
Comment 3 User image Dão Gottwald [:dao] 2012-01-13 08:44:02 PST
How about differentiating between input events that add a single character and those that add multiple characters?
Comment 4 User image :Gavin Sharp [email:] 2012-01-13 14:53:22 PST
Comment on attachment 588430 [details] [diff] [review]

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>+      <field name="_pasteController"><![CDATA[

>+          isCommandEnabled: function(aCommand) {
>+            return this.urlbar.inputField.editor.canPaste(Ci.nsIClipboard.kGlobalClipboard);

The nsPasteCommand::isCommandEnabled implementation seems to also check editor.isSelectionEditable, I wonder whether that's necessary?

This doesn't really seem like a hack to me. It should be reliable, right? It would be nice to fix this problem for all autocomplete widgets, though (wait longer before starting the autocomplete searches triggered by pastes).
Comment 5 User image Marco Bonardo [::mak] 2012-01-16 08:12:05 PST
Created attachment 588888 [details] [diff] [review]

would something like this be fine? It delays autocomplete on paste by 1 second.
I should still make a test.
Comment 6 User image Marco Bonardo [::mak] 2012-01-17 05:30:55 PST
I tried making a chrome mochitest witha type=autocomplete textbox, but the controller complains inputField.editor is null, I wonder if there's a privileges problem, the scope looks correct.
Comment 7 User image Marco Bonardo [::mak] 2012-01-17 05:55:30 PST
now it works, moving on.
Comment 8 User image Marco Bonardo [::mak] 2012-01-17 06:08:56 PST
Created attachment 589169 [details] [diff] [review]
patch v1.0

There are still some things to define:
- if the idl changes may be fine
- if the delay should be configurable and how (pref/attribute)
Comment 9 User image Marco Bonardo [::mak] 2012-01-18 07:39:09 PST
Comment on attachment 589169 [details] [diff] [review]
patch v1.0

new patch coming, according to irc discussion with Gavin.
Comment 10 User image Marco Bonardo [::mak] 2012-01-18 07:46:09 PST
Created attachment 589505 [details] [diff] [review]
patch v2.0
Comment 11 User image :Gavin Sharp [email:] 2012-01-18 11:01:32 PST
Comment on attachment 589505 [details] [diff] [review]
patch v2.0

>diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml

>+      <field name="_pasteController"><![CDATA[

>+          _clipboard: Components.interfaces.nsIClipboard.kGlobalClipboard,

"_kGlobalClipboard"? just "_clipboard" is misleading.
Comment 12 User image Marco Bonardo [::mak] 2012-01-19 05:04:15 PST
addressed comment
Comment 13 User image Matt Brubeck (:mbrubeck) 2012-01-19 10:59:59 PST
Comment 14 User image Roman 2014-06-08 11:35:18 PDT
This seems to have regressed; filed bug 1022399.

Note You need to log in before you can comment on or make changes to this bug.