642615 - If I paste over an auto-suggestion in web console, the suggested text remains

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Daniel Holbert [:dholbert]]

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]]

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)]

unable to reproduce this on a mac. Can someone with linux test?

Comment 3

[Mihai Sucan [:msucan]]

I can reproduce the issue on my Ubuntu Linux.

Comment 4

[Daniel Holbert [:dholbert]]

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]]

Attachment: proposed fix and mochitest

This is the proposed fix and mochitest. Feedback is welcome!

Comment 6

[Kevin Dangoor]

Does this happen to fix bug 627710 as well?

Comment 7

[Mihai Sucan [:msucan]]

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)]

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)]

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]]

(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]]

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/[e-mail]-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]]

Attachment: 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]]

The same failures with the updated patch:

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

Comment 14

[Rob Campbell [:rc] (:robcee)]

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]]

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)]

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]]

I'll do the changes with bug 585991 in mind.

Comment 18

[Mihai Sucan [:msucan]]

Attachment: 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]]

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)]

+
+      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]]

Attachment: 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)]

Comment on attachment 524244 [details] [diff] [review]
patch update 4

yay!

Comment 23

[Rob Campbell [:rc] (:robcee)]

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: [e-mail]]]

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]

(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]]

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: [e-mail]]]

(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: [e-mail]]]

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]]

(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]]

Attachment: 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]]

Can you explain why this patch fixes the bug?

Comment 32

[Dave Townsend [:mossop]]

Also for my own interest, why do you need to manually handle the ctrl-e/a cases there?

Comment 33

[Mihai Sucan [:msucan]]

(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]]

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]]

(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]]

I think it's worth testing both in this case.

Comment 37

[Mihai Sucan [:msucan]]

Attachment: [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]]

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)]

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]]

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