CM6: Merging lint and hover tooltips over the same range

v6
#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.

#10

Hi! Finally found some focus to do some tests.

However setup ended up being somewhat convoluted, given hover relies on dom events, and given I want to check rendering. I ended up basically mocking the view:

Overall it works quite well. But before I add more tests, I would like to confirm if this approach is acceptable, or if there is an easier one. I could try to do selenium, but tbh even with all the effort I still quite like running it in node – it is easier to debug and more deterministic.

I am also basically doing my own DOM mock – I could use jsdom instead if that’s OK.

#11

That is indeed a bit more mocking that I’m comfortable with—I agree that testing with node is much nicer, but this kind of stuff gets very brittle and awkward. Seeing how I still haven’t created a convenient way to run selenium tests in packages other than view, that problem is kind of my fault though. I’ll try to take some time to look into that today.

Other than that, the code looks good now. I do think the way hover tooltips are displayed should be mentioned in the doc comment for hoverTooltip.

#12

Thanks! Updated the doc (and fixed few missing classes).

I’ll wait for selenium test foundation then. I wonder if it still makes sense to use sinon timers even with selenium, given how hover relies on specific timing in a few places.

#13

If you upgrade @codemirror/buildhelper to 0.1.5 and rebase on the current main branch of @codemirror/tooltip, you should be able to create a test/webtest-hover.ts or similar (only the prefix in front of the dash is necessary), and it should automatically be ran in selenium when you do npm test.

#14

Thanks! It actually works way more reliably than I expected, even without sinon (esmoduleserve can’t load sinon either way).

I think PR is ready for review now.

Just a note – this probably needs changes to lint (here: lint/lint.ts at main · codemirror/lint · GitHub) because .cm-tooltip-lint is now on .cm-hover-tooltip-section instead of .cm-tooltip. What’s the best way to release this so that lint is not broken by the tooltip release?

Btw, should it be cm-hover-tooltip or cm-tooltip-hover in the class names?