Adding options customizing a behavior of show-hint addon

[slepowronski]

When working with the show-hint addon (very valuable in general, thanks!), I've come across situations when I had to customize its behavior.

I didn't want to highly customize the internal logic so I was able to simply add options modifying some of the taken actions.

However, I've made also two improvements not being behind the options:

• computing startScroll in setTimeout so it doesn't cause the forced reflow on init
• added a condition over the this.scrollToActive() call as it doesn't need to be scrolled to the first hint on init (especially that selectedHint is 0 by default) and it causes the reflow as well

I'd divide the options into two groups: functional and styling ones (having a performance impact).

Functional:

• closeOnCursorActivity - disable closing the suggestions list on the cursor activity. The suggestions ul element has been "blinking" on every cursor movement what wasn't desired
• closeOnPick - disable closing the suggestions list on picking the item. In my use case, a user picks the suggestion, and the new suggestions list loads immediately based on the previously chosen one so, again, list blinking wasn't desired

Styling:

• paddingForScrollbar - disable adding an additional padding for the scrollbar. Computing that has been causing a very noticeable (at least in our case) force reflows by using things like clientHeight or getBoundingClientRect(). I'd prefer to disable this in order to gain performance
• moveOnOverlap - disable moving the list on overlapping the screen (horizontal or vertical). It computes hints.getBoundingClientRect() what was also heavy performance-wise in our case. We don't need this functionality so I'd prefer to disable it

All of the options above are enabled by default so they don't affect existing users of the addon in any way.

In my particular scenario, I was able to decrease the time spent in the show-hint addon from (on average) > 100ms to max 11ms.

If you'd find these changes valuable, I'd be happy to merge them.

Question: these options need to be described in https://codemirror.net/doc/manual.html#addon_show-hint or it's not strictly required?

Thank you.

[marijnh]

How will the dialog continue to update when, on cursoractivity, it simply doesn't close, but takes no further action?

The thing you call paddingForScrollbar doesn't really seem to be related to padding at all, but rather required for accurately repositioning the dialog—treating the start scroll position as zero will simply produce incorrect results. It'd probably be a better idea to optimize this, for example by initializing it in a timeout to avoid an extra reflow, rather than to just disable it.

To address the lint failure, you can add a // declare global: DOMRect comment somewhere in the file.

[slepowronski]

@marijnh In my case, I'm taking care of updating the show-hint on my own when anything changes in the CodeMirror (using react-codemirror2). I'm triggering the logic of computing the suggestions and finally calling showHint() on every value/cursor change.

My issue with the current implementation was that it has been closing the dialog and opening it again after a fraction of the second. The result perceived by the user has been a flickering of the dialog so I had to avoid that.

Then, you can say that, with closeOnCursorActivity = false and closeOnPick = false, you fully control when the dropdown should be opened/hidden using the showHint()/closeHint() functions.

I've called it paddingForScrollbar because I'm using this flag too for:

var scrolls = completion.options.paddingForScrollbar ? hints.scrollHeight > hints.clientHeight + 1 : false;
...
if (scrolls) for (var node = hints.firstChild; node; node = node.nextSibling)
      node.style.paddingRight = cm.display.nativeBarWidth + "px"

so this is about the right padding in order to compensate for the scrollbar.

However, agree that it's not the case for:

var startScroll = completion.options.paddingForScrollbar ? cm.getScrollInfo() : { ... };

so I've moved startScroll to setTimeout instead.

I've also added a check over the this.scrollToActive() call as it's not required if the first item is active on init (default case) and it has expensive parts like this.hints.clientHeight in its logic.

Please let me know what you think.

P.S. I've added // declare global: DOMRect, thanks.

[slepowronski]

@marijnh I've also updated the description in order to reflect the current state of the PR. Thanks a lot for your feedback.

[marijnh]

Thanks for addressing those. I've merged this. When a regression is reported I'll ping you ;)

[slepowronski]

Amazing, thanks a lot 🥳 Sure, in case of any errors, just let me know and I'll fix it 🙂

Should we add a description for the new options to the docs?

[marijnh]

I'm not sure—they seem error-prone enough to be more likely to confuse and trip-up users than to help.

[slepowronski]

Sure, makes sense. You'd need to be aware that with closeOnCursorActivity / closeOnPick as false, controlling the addon is on your side.

paddingForScrollbar is probably too specific so it doesn't fit the high-level options.

Maybe moveOnOverlap would be okay to put there as it controls the high-level behavior. In some cases, you don't want to have the dialog moved but rather always aligned to the cursor/token.