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

VERIFIED FIXED in Firefox 6

Status

()

Firefox
Developer Tools
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: dholbert, Assigned: msucan)

Tracking

Trunk
Firefox 6
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patchclean:0518][fixed-in-devtools][merged-to-mozilla-central])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
This is using latest mozilla-central nightly:
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b13pre) Gecko/20110317 Firefox/4.0b13pre
unable to reproduce this on a mac. Can someone with linux test?
(Assignee)

Comment 3

7 years ago
I can reproduce the issue on my Ubuntu Linux.
(Reporter)

Comment 4

7 years ago
Also: after Step 3, if you hit "Ctrl Z" (undo) twice, then your typed text all disappears (good), but the light-grey text remains.
(Assignee)

Comment 5

7 years ago
Created attachment 521000 [details] [diff] [review]
proposed fix and mochitest

This is the proposed fix and mochitest. Feedback is welcome!
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attachment #521000 - Flags: feedback?(rcampbell)

Comment 6

7 years ago
Does this happen to fix bug 627710 as well?
(Assignee)

Comment 7

7 years ago
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 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. :)
Attachment #521000 - Flags: feedback?(rcampbell) → feedback+
Comment on attachment 521000 [details] [diff] [review]
proposed fix and mochitest

adding r+ for this, pending a successful try run.
Attachment #521000 - Flags: review+
(Assignee)

Comment 10

7 years ago
(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.
(Assignee)

Comment 11

7 years ago
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.
(Assignee)

Comment 12

7 years ago
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.
Attachment #521000 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Whiteboard: [patchclean:0401]
(Assignee)

Comment 13

7 years ago
The same failures with the updated patch:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=14779f8c75e6
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.
Attachment #521000 - Flags: review+ → review-
(Assignee)

Comment 15

7 years ago
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?
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.
(Assignee)

Comment 17

7 years ago
I'll do the changes with bug 585991 in mind.
(Assignee)

Comment 18

7 years ago
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.
Attachment #523656 - Attachment is obsolete: true
Attachment #524076 - Flags: review?(rcampbell)
(Assignee)

Updated

7 years ago
Whiteboard: [patchclean:0401] → [patchclean:0405]
(Assignee)

Comment 19

7 years ago
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!
+
+      EventUtils.synthesizeKey("Z", {ctrlKey: true});
+    }, false);
+
+    EventUtils.synthesizeKey("V", {ctrlKey: true});

you want accelKey: true instead of explicitly looking for ctrlKey.
(Assignee)

Comment 21

7 years ago
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
Attachment #524076 - Attachment is obsolete: true
Attachment #524076 - Flags: review?(rcampbell)
Attachment #524244 - Flags: review?(rcampbell)
(Assignee)

Updated

7 years ago
Whiteboard: [patchclean:0405] → [patchclean:0406]
Comment on attachment 524244 [details] [diff] [review]
patch update 4

yay!
Attachment #524244 - Flags: review?(rcampbell) → review+
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.
Attachment #524244 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Blocks: 585991
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.
(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...
(Assignee)

Comment 26

6 years ago
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!
(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 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.
Attachment #524244 - Flags: review?(gavin.sharp)
(Assignee)

Comment 29

6 years ago
(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!
(Assignee)

Comment 30

6 years ago
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!
Attachment #524244 - Attachment is obsolete: true
Attachment #531876 - Flags: review?(dtownsend)
(Assignee)

Updated

6 years ago
Whiteboard: [patchclean:0406] → [patchclean:0512]
Can you explain why this patch fixes the bug?
Also for my own interest, why do you need to manually handle the ctrl-e/a cases there?
(Assignee)

Comment 33

6 years ago
(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 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.
Attachment #531876 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 35

6 years ago
(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+!
I think it's worth testing both in this case.
(Assignee)

Comment 37

6 years ago
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!
Attachment #531876 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Whiteboard: [patchclean:0512] → [patchclean:0512][checkin-needed]
(Assignee)

Comment 38

6 years ago
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
Attachment #532961 - Attachment description: patch update 6 → [in-devtools] patch update 6
(Assignee)

Updated

6 years ago
Whiteboard: [patchclean:0512][checkin-needed] → [patchclean:0518][fixed-in-devtools]

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [patchclean:0518][fixed-in-devtools] → [patchclean:0518][fixed-in-devtools][merged-to-mozilla-central]
Target Milestone: --- → Firefox 6

Comment 39

6 years ago
Comment on attachment 532961 [details] [diff] [review]
[checked-in] [in-devtools] patch update 6

http://hg.mozilla.org/mozilla-central/rev/d86add1abd34
Attachment #532961 - Attachment description: [in-devtools] patch update 6 → [checked-in] [in-devtools] patch update 6

Comment 40

6 years ago
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.