CM6: Merging lint and hover tooltips over the same range

/next
#1

Hi! I was implementing a hoverTooltip when I noticed that it overlaps with the lint one:
image

I think it would be nicer to somehow tell all tooltips over the same range to merge their DOM into one container, similar how VS Code does it here:

image
image1245×221 28.8 KB

Is there a way to do this in the current version?

#2

Downright merging tooltips would require changing the interface, but making sure they aren’t placed over each other is a good idea and not very invasive. Does this patch look like it’d solve your issue?

#3

Thanks – I’ll try it!

As for the interface, I thought it could be done without changing it?
TooltipView.dom can be added to the same container as other doms, mount can be called after that, positioned can be called based on the container.

I’ll try to prototype something – I have seen few more editors that merge tooltips since, so I think it’s worth at least trying out.

#4

I was thinking about the way people style their tooltips (see this example)—that kind of thing doesn’t really work anymore when tooltips are supposed to just be uniform blocks inside bigger elements.

#5

That’s fair. But merging does not have to be the only option, maybe something like Tooltip.allowsMerging to allow it? (as long as lint tooltips have that set by default)

#6

OK, here is a draft approach to mergeable (“hosted”) tooltips:

I simplified the original idea a bit:

  1. This is only for hoverTooltip (parameter “hosted”)
  2. The layout is always predictable – either hosted (in container) or not hosted – this is not a dynamic merge. All hosted tooltips are merged, all non-hosted tooltips are not.

Things I haven’t looked at yet (until initial approach is confirmed):

  1. Tests – I tested demo manually, but obviously it’s not good enough
  2. Documentation/comments
  3. Handling above and strictSide in some way – could do overloads to hoverTooltip that statically block those if hosted is true, but the typing might get confusing for people.
  4. Lint PR – this is trivial, just need to add hosted: true and remove .cm-tooltip requirement from one of the styles.

Please let me know if this direction makes sense, and if it does, I can continue on the remaining work.

#7

If this only applies to hover tooltips, I’d be okay with making it the only behavior for hover tooltips. We could make above act like a hint (i.e. ignore strictSide) and, when grouping, just take the side of the first tooltip. That would keep the types from getting too complicated.

As for the DOM structure, I think if we document the fact hover tooltips nest inside an additional element, we’re fine. Not sure what to do by default for borders between the elements. I guess the base theme could assign a simple border like in your screenshot.

Regarding the code, I think the tooltip-managing logic (as in updateTooltip) would best be expressed as a class, so that we don’t have to push 6 input parameters and 4 return values back and forth like that.

#8

Thanks! I applied class suggestion (code is way nicer) and made hosting unconditional for hover tooltips. Didn’t have time to look at the borders or tests yet, but hope to find some time soon.

#9

Nice. Let me know when it’s ready to review.