remove extra margins on completion hovers with markdown

[agurod42]

Fixes #59036

[ramya-rao-a]

We need those margins for docs that have multiple paragraphs.

Below is one such case:

Below is the same case with the changes in this PR

Set sticky to true. Then in the local instance of VS Code, run Help -> Toggle Developer Tools -> Elements`

So, we have to

• Remove the margin top only on the first <p>, and margin bottom on the last <p>
• Add margin-bottom to the element above the docs

[agurod42]

Hi @ramya-rao-a,

Loved the sticky trick. Thank you!

I made the changes you requested:

[ramya-rao-a]

Well... now the bottom is clipped off

[ramya-rao-a]

Now there is no margin between paragraphs when there are only 2 paragraphs..

Before:

After:

Instead of setting margin:0 on the first and last child, try setting margin-top:0 on the first child and margin-bottom:0 on the last child.

[agurod42]

Well... It was harder than it seemed 😅

Now I think it working as expected:

header + one paragraph:

header + two paragraphs:

header + more than two paragraphs

header only

no header, no markdown

no header, markdown

I was forced to add a class when rendering the widget in order to be able to remove the header padding when the docs section is empty. I think it shouldn't be a problem.

Thanks for your help!

[ramya-rao-a]

Is the assumption that this.docs.innerText is empty when markdown is being used?

[agurod42]

Yes. this.docs refers to the p element which eventually has the markdown-docs class. It's innerText property will be empty if there is no content to show (Just like in the case shown in the image below).

[ramya-rao-a]

I am hesitant to add this class. On a cursory read of the code, it would lead one to believe that there are no "docs" associated with the suggestion item. In the suggestWidget file docs is referring to the part that appears on the right of the suggestion list.

I actually liked the change you did in your previous commit of increasing the bottom padding of .type all the time. Wouldnt that be enough (plus the margin-top/margin-bottom on first/last child)?

[agurod42]

Yeahp. You're right.

The problem with increasing the padding of .type all the time arises when the .docs item doesn't have content:

Here the space below the header should be smaller. See current (v1.28.0-431ef9d) aspect:

I think I just found an approach that doesn't need the empty-docs class.

[agurod42]

@ramya-rao-a I think the last commit solves this last issue without adding the empty-docs class. All the cases detailed in this comment #59786 (comment) are still working.

PS. Do you mind if I squash the commits in this PR?

[soleo]

Just curious, why do you need to use a specific rule like
.docs.markdown-docs > div > p:last-child? isn't it sufficient enough to have Just curious, why do you need to use a specific rule like .docs.markdown-docs p:last-child?

[ramya-rao-a]

@soleo I think there was some copy/paste problem with your comment above

[ramya-rao-a]

Last question :)

Why are we moving the padding from .docs.markdown-docs to .docs.markdown-docs > div?

[agurod42]

Hehe, no problem!

I'll try to explain it:

Problem:

1. We need this padding-bottom in order to provide space between the widget's header and content:

2. We also need the .markdown-docs padding to provide space between the content and the widget borders:

3. When there is no content to show, the .markdown-docs padding generates this unwanted effect:

Solution:

When the suggestion docs has markdown content, this function is being called in order to render it's contents:

https://github.com/Microsoft/vscode/blob/e8edae54aa99d06ddb37211bebc6faffb20ff1fb/src/vs/editor/contrib/suggest/suggestWidget.ts#L262

This one ends up calling the createElement in the htmlContentRenderer class, which eventually generates the html element that will be added as the .markdown-docs children:

https://github.com/Microsoft/vscode/blob/e8edae54aa99d06ddb37211bebc6faffb20ff1fb/src/vs/base/browser/htmlContentRenderer.ts#L30

When there is no content to show, the options.inline property becomes true and an empty span is added to the .markdown-docs. Here we shouldn't add padding.

In order to consider this scenario and avoid adding the problematic padding we can apply the padding to the .markdown-docs's immediate children instead of applying it to the .markdown-docs element itself. To do that we need to differentiate two cases:

1. the children is a div element and we should always add padding
2. the children is a span element and we should only add padding it the span is not empty (I didn't found any suggestion widget with markdown-docs and a non-empty span as children but I think we should handle it, just in case).

That's why I moved the padding from .docs.markdown-docs to .docs.markdown-docs > div (and also added another rule to handle the span:non(:empty) case).

Hope I was able to explain myself in a clear way.

Kr

[ramya-rao-a]

Thanks @agurodriguez!
I hope you registered for the Microsoft Hactoberfest :)