Last Comment Bug 642615 - If I paste over an auto-suggestion in web console, the suggested text remains
: If I paste over an auto-suggestion in web console, the suggested text remains
Status: VERIFIED FIXED
[patchclean:0518][fixed-in-devtools][...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: Firefox 6
Assigned To: Mihai Sucan [:msucan]
:
:
Mentors:
Depends on:
Blocks: 585991
  Show dependency treegraph
 
Reported: 2011-03-17 15:57 PDT by Daniel Holbert [:dholbert]
Modified: 2011-05-26 04:48 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix and mochitest (5.72 KB, patch)
2011-03-22 12:49 PDT, Mihai Sucan [:msucan]
rcampbell: review-
rcampbell: feedback+
Details | Diff | Splinter Review
updated patch (6.52 KB, patch)
2011-04-01 12:24 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
patch update 3 (9.90 KB, patch)
2011-04-05 10:56 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
patch update 4 (9.96 KB, patch)
2011-04-06 13:23 PDT, Mihai Sucan [:msucan]
rcampbell: review+
Details | Diff | Splinter Review
patch update 5 (12.16 KB, patch)
2011-05-12 02:46 PDT, Mihai Sucan [:msucan]
dtownsend: review+
Details | Diff | Splinter Review
[checked-in] [in-devtools] patch update 6 (12.56 KB, patch)
2011-05-17 08:02 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2011-03-17 15:57:45 PDT
STEPS TO REPRODUCE:
 0. Copy 10-20 characters into your clipboard.  (e.g. this text right here)
 1. Tools | Web Console
 2. Type "de" into the Web Console
     --> "faultStatus" will appear in light-gray
 3. Ctrl+V to paste

EXPECTED RESULTS: light-gray suggestion disappears, (replaced by your paste)
ACTUAL RESUTLTS:  light-gray suggestion remains, with your pasted text on top of it.
Comment 1 Daniel Holbert [:dholbert] 2011-03-17 15:58:13 PDT
This is using latest mozilla-central nightly:
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b13pre) Gecko/20110317 Firefox/4.0b13pre
Comment 2 Rob Campbell [:rc] (:robcee) 2011-03-21 08:48:25 PDT
unable to reproduce this on a mac. Can someone with linux test?
Comment 3 Mihai Sucan [:msucan] 2011-03-21 08:53:16 PDT
I can reproduce the issue on my Ubuntu Linux.
Comment 4 Daniel Holbert [:dholbert] 2011-03-21 09:48:59 PDT
Also: after Step 3, if you hit "Ctrl Z" (undo) twice, then your typed text all disappears (good), but the light-grey text remains.
Comment 5 Mihai Sucan [:msucan] 2011-03-22 12:49:59 PDT
Created attachment 521000 [details] [diff] [review]
proposed fix and mochitest

This is the proposed fix and mochitest. Feedback is welcome!
Comment 6 Kevin Dangoor 2011-03-24 09:40:15 PDT
Does this happen to fix bug 627710 as well?
Comment 7 Mihai Sucan [:msucan] 2011-03-24 09:58:17 PDT
Kevin: I cannot reproduce that bug in mozilla-central.

So, this patch here does not affect that. There should be no problem with changing input focus.

Robert: please try to reproduce the bug again and let us know if that's still valid. Thanks!
Comment 8 Rob Campbell [:rc] (:robcee) 2011-03-31 08:47:49 PDT
Comment on attachment 521000 [details] [diff] [review]
proposed fix and mochitest

+            setTimeout(function() {
+              if (self.inputNode.value !== value) {
+                self.complete(self.COMPLETE_HINT_ONLY);
+              }
+            }, 0);

why is the setTimeout needed here?

...

after staring at it for a bit, I'm guessing the inputNode's value isn't getting set quickly enough for your comparison to work.

in the test file:

+  let HUD = HUDService.hudReferences[hudId];
+  let jsterm = HUD.jsterm;

You could just set jsterm directly from the hudreference lookup, but the extra variable doesn't kill us.

Looks fine, but we should run this through try to see if it breaks. I'd be curious to see if we can eliminate that setTimeout, though it's probably needed. I wonder if you couldn't move the test inside the self.complete() call? Or implement a self.completeIfValue(value) or something?

Y'know what? It's probably fine the way it is. Nice patch. :)
Comment 9 Rob Campbell [:rc] (:robcee) 2011-03-31 13:33:50 PDT
Comment on attachment 521000 [details] [diff] [review]
proposed fix and mochitest

adding r+ for this, pending a successful try run.
Comment 10 Mihai Sucan [:msucan] 2011-04-01 07:28:12 PDT
(In reply to comment #8)
> Comment on attachment 521000 [details] [diff] [review]
> proposed fix and mochitest
> 
> +            setTimeout(function() {
> +              if (self.inputNode.value !== value) {
> +                self.complete(self.COMPLETE_HINT_ONLY);
> +              }
> +            }, 0);
> 
> why is the setTimeout needed here?
> 
> ...
> 
> after staring at it for a bit, I'm guessing the inputNode's value isn't getting
> set quickly enough for your comparison to work.

Yep, that's the problem. That's the keydown event handler - it gets executed before the inputNode is updated. We should actually make some additionally changes to the way we handle keyboard input - the current solution with the timeout is far from ideal. 

> in the test file:
> 
> +  let HUD = HUDService.hudReferences[hudId];
> +  let jsterm = HUD.jsterm;
> 
> You could just set jsterm directly from the hudreference lookup, but the extra
> variable doesn't kill us.
> 
> Looks fine, but we should run this through try to see if it breaks. I'd be
> curious to see if we can eliminate that setTimeout, though it's probably
> needed. I wonder if you couldn't move the test inside the self.complete() call?
> Or implement a self.completeIfValue(value) or something?

It's not something I'd put in complete(). It's more of a problem with the way we handle keyboard events. We should have these calls to complete() in keyup or keypress, not in keydown.

I kept the patch minimal, without any attempts to "make things right" - to keep it easy for review. A better keyboard event handling approach would be a good cleanup bug. Or perhaps that's not going to be needed since we'll switch GCLI, right?

> Y'know what? It's probably fine the way it is. Nice patch. :)

Thanks for the feedback+ and review+! :)

Will push the patch to the try server.
Comment 11 Mihai Sucan [:msucan] 2011-04-01 11:32:33 PDT
The try push results are not good. The new test fails in debug and opt builds on Mac and there's a crasher. Windows results are still pending.

Builds available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mihai.sucan@gmail.com-928a8051f173

Changeset:
http://hg.mozilla.org/try/pushloghtml?changeset=928a8051f173

Test results:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=928a8051f173

I'm going to look into the patch and see if I can fix it, but I don't know yet what may cause the failure.
Comment 12 Mihai Sucan [:msucan] 2011-04-01 12:24:20 PDT
Created attachment 523656 [details] [diff] [review]
updated patch

After investigation it seems that the problem is caused by our keydown event handler that calls complete() in a setTimeout(fn, 0) - it waits for the inputNode.value to be updated. Also, the new test waits for the complete() execution result ... with an executeSoon() call. Normally, it should all happen in keyup event handlers, but for that we'd need to rip off the keydown event handler in jsterm and properly rewrite it all - better keyboard handling than there's now in the code. Such invasive change would also allow us to write a better test.

The updated patch does listen for keyup and executes the test event handlers ... in executeSoon() ... in the hope that all these delays are enough for correct execution order. Will push this patch to the try server.

If this fails, we are only left with the choice of disabling the test or ... rewrite the keyboard handling code in jsterm. Still, GCLI will come with its own stuff anyway. Not sure what to do - if GCLI is scheduled for Fx5 then we should aim for that.
Comment 13 Mihai Sucan [:msucan] 2011-04-02 01:17:34 PDT
The same failures with the updated patch:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=14779f8c75e6
Comment 14 Rob Campbell [:rc] (:robcee) 2011-04-02 07:21:32 PDT
This result is exactly what I was afraid of with the setTimeout(x, 0) in your handler.

Creating a completely custom keyboard handler for the JSTerm is probably unavoidable, but it's risky to attempt it for this bug, I think.

Not sure I like the idea of disabling the tests for this as we're hiding bad results in an area of code that is going to be prone to failure in the real world. I think we need another solution.
Comment 15 Mihai Sucan [:msucan] 2011-04-02 07:36:13 PDT
Yep, I was afraid of that as well (the timeouts), but I was made to believe it works, seeing tests pass in opt and debug builds on my system.

A completely custom keyboard handler for the JSTerm is what we have now, actually. It's just not very well implemented. Or "completely custom keyboard handler" means something different for you. :)

What I propose is just fix the current code, with a little more invasive changes to the keydown event handler, and some new keyboard event handlers (keyup, "change" and/or "input"). Does that sound fine for you?
Comment 16 Rob Campbell [:rc] (:robcee) 2011-04-02 17:41:15 PDT
Yup, I think we need to be a little more rigourous with our keyboard handling here. Not much choice, though managing keyup/keydown state will get a little complicated when we start tacking on features like the autocompleter rewrite in bug 585991.
Comment 17 Mihai Sucan [:msucan] 2011-04-03 01:24:34 PDT
I'll do the changes with bug 585991 in mind.
Comment 18 Mihai Sucan [:msucan] 2011-04-05 10:56:13 PDT
Created attachment 524076 [details] [diff] [review]
patch update 3

Updated the patch. Now keyboard handling does not involve any use of setTimeout(), and it wasn't a too invasive change. Things should be clearer, hopefully.

All the tests continue to pass for me. Will push this patch to the try server.
Comment 19 Mihai Sucan [:msucan] 2011-04-06 01:07:17 PDT
Try server results:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=f08507d66510

Negative on Macs, again.

This time I doubt it's a problem with the HUDService code. I am quite certain the code is fine.

There's a bug with Firefox on Macs, or Ctrl-V doesn't do what I expect it to do ... on Macs. Thoughts?

Rob: can you please check if the synthesized key (Ctrl-V) actually works? In the mochitest, on your Mac. Thanks!
Comment 20 Rob Campbell [:rc] (:robcee) 2011-04-06 12:05:13 PDT
+
+      EventUtils.synthesizeKey("Z", {ctrlKey: true});
+    }, false);
+
+    EventUtils.synthesizeKey("V", {ctrlKey: true});

you want accelKey: true instead of explicitly looking for ctrlKey.
Comment 21 Mihai Sucan [:msucan] 2011-04-06 13:23:04 PDT
Created attachment 524244 [details] [diff] [review]
patch update 4

Updated patch. No longer using synthesized keys for paste and undo. I now call the commands.

The test now seems to pass on the try server as well. See results here:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=a01c4f1dcce9
Comment 22 Rob Campbell [:rc] (:robcee) 2011-04-08 14:00:50 PDT
Comment on attachment 524244 [details] [diff] [review]
patch update 4

yay!
Comment 23 Rob Campbell [:rc] (:robcee) 2011-04-08 14:02:07 PDT
Comment on attachment 524244 [details] [diff] [review]
patch update 4

requesting additional review for this. Already tested and reviewed, but feel free to give it a once over.
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-19 16:48:02 PDT
This code looks overly complicated - why doesn't handleKeyPress live directly on the object, rather than going through keyPress()? You can bind() it to "this" when passing to addEventListener.
Comment 25 Shawn Wilsher :sdwilsh 2011-04-19 17:05:20 PDT
(In reply to comment #24)
> This code looks overly complicated - why doesn't handleKeyPress live directly
> on the object, rather than going through keyPress()? You can bind() it to
> "this" when passing to addEventListener.
Mossop has just asked for bind to not be used with event handlers in bug 585991...
Comment 26 Mihai Sucan [:msucan] 2011-04-20 06:21:19 PDT
Gavin: That's how the original code author did it, and now that Mossop said I shouldn't do bind(), I don't know what's preferred.

I personally favor bind(), but no problem: I can change the patch as you request.

Thanks for looking into the patch!
Comment 27 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-20 11:34:49 PDT
(In reply to comment #25)
> Mossop has just asked for bind to not be used with event handlers in bug
> 585991...

Cleared this up on IRC - he just doesn't like replacing the property with a bound method. Passing the bound method to addEventListener is fine, and we can do that here.
Comment 28 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-11 17:53:01 PDT
Comment on attachment 524244 [details] [diff] [review]
patch update 4

I'm not going to be able to find the time to understand all of this completion  code in the near future. Also I would like to see it be cleaned up as mentioned above. I don't think either of those necessarily need to blcok this patch.
Comment 29 Mihai Sucan [:msucan] 2011-05-12 02:38:54 PDT
(In reply to comment #28)
> Comment on attachment 524244 [details] [diff] [review] [review]
> patch update 4
> 
> I'm not going to be able to find the time to understand all of this
> completion  code in the near future. Also I would like to see it be cleaned
> up as mentioned above. I don't think either of those necessarily need to
> blcok this patch.

No problem.

I am going to update the patch to clean that part. I stayed away from the (minor) cleanup because it would look like a rewrite of the two methods in question - while it's only a code re-indentation and s/self/this/g.

Will ask Mossop for review because he has already reviewed the autocomplete popup stuff (bug 585991) - hopefully that makes reviewing this patch easier for him.

Thank you!
Comment 30 Mihai Sucan [:msucan] 2011-05-12 02:46:04 PDT
Created attachment 531876 [details] [diff] [review]
patch update 5

Updated the patch per comment 24.

Dave: Based on comment 28 I am asking you for review. You have already reviewed the autocomplete popup patch (bug 585991). Thank you!
Comment 31 Dave Townsend [:mossop] 2011-05-12 13:43:15 PDT
Can you explain why this patch fixes the bug?
Comment 32 Dave Townsend [:mossop] 2011-05-12 13:45:53 PDT
Also for my own interest, why do you need to manually handle the ctrl-e/a cases there?
Comment 33 Mihai Sucan [:msucan] 2011-05-16 11:44:13 PDT
(In reply to comment #31)
> Can you explain why this patch fixes the bug?

Unfortunately the patch grew bigger and less obvious due to the changes in indentation and the use of .bind().

Problem was that we had:

- a keyDown() method which was actually a keypress event handler. This method dealt with ctrl+something and any other key without ctrl down.

Autocomplete suggestions were updated only when Ctrl was not active. For Ctrl-X/C/V that's already a failure, because these keys change the input. Still, the code assumed they don't.

- an input event handler which only resizes the input.


The patch attached does:

- renames keyDown() to keyPress().

- adds logic in the input event handler to update the autocomplete result, not just resize the input. This now works with ctrl-x/c/v and other cases which change the input.

- removes the autocomplete update logic from keyPress() since they are now all handled by the input event handler.

- remove unneeded delays in the keyboard event handling code.

(In reply to comment #32)
> Also for my own interest, why do you need to manually handle the ctrl-e/a
> cases there?

We want our input to have a more commandline-like feeling. This is "old" code that I didn't really touch, I just removed the unneeded timeouts/delays.


Thank you for your questions!

Please note that we would like to land this patch in Firefox 6. Thanks!
Comment 34 Dave Townsend [:mossop] 2011-05-16 13:50:12 PDT
Comment on attachment 531876 [details] [diff] [review]
patch update 5

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

Any reason you gave up on trying to synthesize the keyboard event for pasting? For something so tied up in keyboard handling seems like we should test that case too.
Comment 35 Mihai Sucan [:msucan] 2011-05-16 13:55:29 PDT
(In reply to comment #34)
> Comment on attachment 531876 [details] [diff] [review] [review]
> patch update 5
> 
> Review of attachment 531876 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Any reason you gave up on trying to synthesize the keyboard event for
> pasting? For something so tied up in keyboard handling seems like we should
> test that case too.

I thought that testing for cmd_paste was more general, than Ctrl-V, since cmd_paste is executed by Ctrl-V (think of a click in the Edit menu). If you want I can add that keyboard event to the test as well.

Thanks for your review+!
Comment 36 Dave Townsend [:mossop] 2011-05-16 13:57:24 PDT
I think it's worth testing both in this case.
Comment 37 Mihai Sucan [:msucan] 2011-05-17 08:02:50 PDT
Created attachment 532961 [details] [diff] [review]
[checked-in] [in-devtools] patch update 6

Updated the test to include Ctrl-V as well, as requested by Dave. Thanks!
Comment 38 Mihai Sucan [:msucan] 2011-05-18 05:51:30 PDT
Comment on attachment 532961 [details] [diff] [review]
[checked-in] [in-devtools] patch update 6

Pushed the patch to the devtools repo:

http://hg.mozilla.org/projects/devtools/rev/d86add1abd34
Comment 39 Dave Camp (:dcamp) 2011-05-21 21:18:50 PDT
Comment on attachment 532961 [details] [diff] [review]
[checked-in] [in-devtools] patch update 6

http://hg.mozilla.org/mozilla-central/rev/d86add1abd34
Comment 40 AndreiD[QA] 2011-05-26 04:48:25 PDT
Verified fixed on:
Windows 7:
Mozilla/5.0 (Windows NT 6.1; rv:6.0a2) Gecko/20110525 Firefox/6.0a2
Window XP:
Mozilla/5.0 (Windows NT 5.1; rv:6.0a2) Gecko/20110525 Firefox/6.0a2
Mac OS 10.6
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a2) Gecko/20110525 Firefox/6.0a2 
Linux i686:
Mozilla/5.0 (X11; Linux i686; rv:6.0a2) Gecko/20110525 Firefox/6.0a2

*Note: If pasting a text in the web console the auto suggest disappears. Markign this bug as Verified

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