Fix inconsistent popup's bottom margins

[rchl]

ST can't margin-collapse margins of nested elements so elements with
bottom-margin that were direct children of the .lsp_popup element would
add an extra margin to the popup's bottom side.

Fix by instead always appending a spacer element with top margin at the
end of the popup contents. ST is able to margin-collapse margins of
sibling elements so that should create a consistent margin, regardless
of the content.

Fixed by creating a helper functions for showing and updating popups
that always add the spacer element. Those also have defaults for some
common arguments so that we don't have to always pass them.

One additional logic change, apart from adding the spacer element is
that we default to max_height=800 instead of the default max-height=240.

Resolves #1553

[rchl]

I see it not working in some cases so will have to investigate more.

[rchl]

It's a slightly different case of the margin-collapsing ST bug that is still not working:

<div class="lsp_popup">
  <div>
    <div class="highlight">
      code block
    </div>
  </div>
  <div class="lsp_popup--spacer"></div>
</div>

The .highlight has margin-bottom: 0.5rem and the extra div wrapper around it doesn't allow the margins to collapse.
The case that is fixed by this fix is when .highlight doesn't have a wrapper element.

[predragnikolic]

This issue was my biggest annoyance lately. Thanks for fixing it. :)

Sure there are cases that this PR won't cover. But if fixed all the issues that I had experienced.

I just found one case that is left.

LSP/plugin/completion.py:43

- content += "<div>{}</div>".format(documentation)
+ content += documentation

before:

after

[rchl]

Yes, that was also where I saw the issue.

Removing the wrapper div will improve things indeed but the problem will still pop up somewhere eventually, depending on the content.

I'll apply your suggestion though and then we can decide if we want this or not.

[predragnikolic]

I'll apply your suggestion though and then we can decide if we want this or not.

There is something in this sentence that makes me think that you are not happy with the implementation?
I mean if you are not, than maybe it is better to leave the bug aside and wait ST to fix it.
Although I am ok with all the changes here.

But if I was more of a critique of the PR I would say:
To me the only "downside" is that we will have show_lsp_popup which calls mdpopups.show_popup which calls view.show_popup. We will have a wrapper for a wrapper. Which is semi ok to me.

The only difference between show_lsp_popup and mdpopups.show_popup is that show_lsp_popup attaches an additional html tag at the bottom of the popup. Which is understandable.

Ideally I thought that we would only modify the css, and that the commit wouldn't be that long and easy revertable once ST fixes it, but this also works.

[rchl]

There is something in this sentence that makes me think that you are not happy with the implementation?

Yeah, I'm not happy that it doesn't fix all cases. I think only the ST fix can really do this.

To me the only "downside" is that we will have show_lsp_popup which calls mdpopups.show_popup which calls view.show_popup. We will have a wrapper for a wrapper. Which is semi ok to me.

I think that's a nice thing about this change since:

• we can ensure more consistency with regards to max_width/max_height
• less stuff to import when wanting to show or update a popup (no need to import mdpopups and .css)
• fewer arguments to pass since we can rely on lsp-specific defaults
• and of course the main point of this change - being able to add an extra spacer element from one place only.

Ideally I thought that we would only modify the css, and that the commit wouldn't be that long and easy revertable once ST fixes it, but this also works.

I don't see a way to achieve a similar level of improvement with just using CSS. Your suggestions from the bug would apply in too many cases, removing the bottom margin from adjacent elements like code blocks or paragraphs.

BTW. This workaround wouldn't break anything once ST fixes the margin-collapsing issue so we wouldn't need to rush to remove it.

[predragnikolic]

I don't see a way to achieve a similar level of improvement with just using CSS

True

This workaround wouldn't break anything once ST fixes it

And true

As for the "fixing all the possible case" while that is probably posslibe with more complex solutions. Those soultion would also not guarantee that we will still miss a few cases here and there.

That said, I think that this solution is more than ok.

[rwols]

Some tiny nitpicks.

[rwols]

The type is Optional[Callable] for on_navigate and on_hide. I don't understand why mypy does not complain about this.

[rchl]

It understands that it's optional because it has a default value (=None) which "modifies" (or augments) the type.

I can change it if you prefer it more explicit (and longer). I forgot the style I'm supposed to use due to not touching the code in a while. ;)

[rwols]

Alright let's see how it behaves :)