UX issues with tooltips

[alexristich]

Four issues:

1) I noticed a flicker when clicking from within the body of one function to anywhere else in a program.

For example:

point(clientX, clientY);
image(getImage("avatars/leaf-blue"), 0, 0, 200, 200);

Say my cursor is between the "g" and "e" of getImage(). If I then click onto one of the parameters of point() then the tooltip for image() will flicker briefly before disappearing.

2) When the cursor is within the body of a function where tooltips are available (and the tooltip is showing), if I click onto a commented line of code, the tooltip doesn't disappear.

3) When writing a function which takes another function as a parameter (such as image() above), once I close the parens for the first function, the tooltip does not show for the rest of the function.

4) When typing on a commented line, the autosuggest feature remains active

Tracking down the first one took a while, as I'm new to the project so a lot of it is just getting used to the structure. I think I've figured out why it's happening, but I have a question.

        setTimeout(function(){
            if (shiftPressed) {
                editor.selection.selectToPosition(pos);
            }
            else if (!this.$clickSelection) {
                editor.selection.moveToPosition(pos);
            }
            this.select();
        }.bind(this), 0);

Is there any reason for the setTimeout() here?

Secondly, I noticed some inconsistencies when using the mouse and keyboard. With the keyboard, if I press left or right, the tooltip appears where the cursor moves to. If I click to the left or right with the mouse, the tooltip does not appear, and only appears on the second click on in that same location. Is the desire to have the tooltip show up right away when clicking with the mouse as well?

Anyway, I'm happy to continue to spend some time on these and will submit a PR with the four fixes once I've managed to get everything figured out.

[kevinbarabash]

I'll have a look at it over the weekend. You could try it without the setTimeout and see what happens.

[alexristich]

Sure thing. I ask actually because I did try it, and removing setTimeout in this case fixes the flicker issue noted in item 1. It also results in a more consistent behavior between the mouse and keyboard, and I think a better user experience as well.

I ran the tests and there seems to be no difference in tests passing/failing when removing this, and couldn't find any cases where anything was breaking any other UI elements when testing through the simple demo. However, since I don't know why this code was added in the first place, I hesitate to remove it without asking any questions!

Also, sorry! I should have been more clear as to where this code is. Otherwise it's not too helpful :) This snippet is from editor_ace_deps.js:2400.

[alexristich]

I just submitted a PR for this one. I haven't yet been able to figure out how to fix the third issue - the editor is behaving differently when the selection is preceded by a ), and this is affecting the event that doRequestTooltip is passing to the emitter. I'm a little stuck here to be honest, so I'm submitting fixes for 1, 2 and 4 for now, and will see if I can track down exactly what's happening further down the stack.

Also, WebStorm was insistent on fiddling with some spacing in the tooltips.js file so there's unfortunately a bit of fluff in the commit.

[kevinbarabash]

If you can't solve 3 right now, that's okay. We can open a separate issue for it. I think the fluff is getting rid of trailing whitespace which is good so it's okay.

[alexristich]

Kevin, I don't think I'm going to solve number 3 any time soon :) I spent another good while at it today without any luck. Here's what I've found so far, using the example below for reference:

image(getImage("avatars/leaf-blue"), 0, 0, 100, 100)

1) If I click on getImage then the tooltip for image shows properly. If I click on the comma after getImage, the tooltip does not show.

2) In the first case, when the event hits doRequestTooltip, params has the property propagationStopped: true. In the second case, this value is unset for params. This means the first one goes through the regular requestTooltip route, whereas the other calls requestTooltipDefaultCallback which hides the tooltip.

3) In the first case, when it hits enableLiveCompletion in autosuggest.js, the value of the parameter enable is set to "false" whereas in the latter case it is set to "true".

There are one or two other instances where I've found the behaviors between the two to be notably different, yet despite these hints I've had no luck poking my way up the call stack to figure out exactly where the two diverge. I also recognize it must be related to the value returned by a check to see whether the parens are open or closed, but I haven't had any luck finding anything related to parens/brackets in the code that, when adjusted, affects this in any way.

If you have any ideas of where I might look from here that would be great, otherwise perhaps you or someone else can swing back onto this one at a later date to pick up from here.

[alexristich]

I take it all back. I couldn't quite let this one die without giving it another shot, and finally managed to figure it out. Turns out the regular expression in auto-suggest.js was a bit "faulty" and wasn't accounting properly for extra closing parens. I tweaked it a bit and also added another check to correctly return when the outer function is closed.

Regular expressions are a pain, as I'm sure you know all too well :) Please feel free to make any additional tweaks here as you feel fit. I've pushed this to my master branch, so it'll be included in the pending PR. Now that I've finished with this string of issues (hopefully!) I'll push any other changes to a new branch as you suggested so as to avoid any dependencies.