Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix display of ?? help #9617

Merged
merged 3 commits into from
Jan 14, 2021
Merged

Fix display of ?? help #9617

merged 3 commits into from
Jan 14, 2021

Conversation

jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jan 14, 2021

References

This is yet another solution to #7760 that avoids the problems we see from the solution in #8555 (noted in #9600).

Fixes #9600

Code changes

In #8555, we tried to solve #7760 by setting spans to have inline-block display. However, this makes the newlines in the spans ignored, i.e., they don't cause linebreaks. This leads to really bad formatting, like noted in #9600.

This reverts #8555 and instead expands the background-colored spans by adding padding above and below to match the line height (as long as the lineheight is a multiple like 1.03 or something).

User-facing changes

Fixes #7760:

Screen Shot 2021-01-13 at 10 30 12 PM

Fixes #9600:
Screen Shot 2021-01-13 at 10 30 22 PM

Another example from https://nbsphinx.readthedocs.io/en/material-theme/code-cells.html#ANSI-Colors:

Screen Shot 2021-01-13 at 10 48 39 PM

Backwards-incompatible changes

…nd color.

This eliminates the gaps between lines where you have an ansi background color, which is caused by the lines having a line height that is slightly larger than the font size so that we have some nice spacing between the lines. When we have ANSI background colors, that spacing becomes really glaring, as noted in jupyterlab#7760.

This is yet another solution to jupyterlab#7760 that avoids the problems we see from the solution in jupyterlab#8555 (noted in jupyterlab#9600).

Fixes jupyterlab#9600
@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jasongrout jasongrout added this to the 3.0 milestone Jan 14, 2021
@github-actions github-actions bot added Design System CSS pkg:rendermime tag:CSS For general CSS related issues and pecadilloes labels Jan 14, 2021
Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@blink1073 blink1073 merged commit 4c14983 into jupyterlab:master Jan 14, 2021
@dakoop
Copy link
Contributor

dakoop commented Jan 14, 2021

Like this approach, but I think the padding calculation may be incorrect. If so, this is clearly a minor issue compared to what the fix addresses. I haven't checked on latest 3.0.x branches, but going back to my branch for this issue and making similar changes, it looks like the padding-top of one line overlaps the padding-bottom of the previous one. If you compare the screenshot above with the one in #8555, you can see the difference on the bottom line. I believe you can also see this in the third example above (the height above "BEWARE" is greater than the height above the previous line because the first line is being clipped by the containing element). This is harder to see in the original ansi colors test from #7760 because the background colors are all the same vertically. Here is a modified test (swapping rows and columns) to better show top and bottom padding.

text = ' XYZ '
formatstring = '\x1b[{}m' + text + '\x1b[m'

print(' ' * 4 +
      ''.join(' {:>4}'.format(('1;' if bold else '') + str(fg)) for fg in range(30, 38) for bold in [False, True]))
for bg in range(40, 48):
    print('{:^{}}'.format(bg, len(text)) +
          ''.join(formatstring.format(('1;' if bold else '') + str(fg) + ';' + str(bg))
                  for fg in range(30, 38) for bold in [False, True]))

Screenshot using devtools shows the padding overlap:

Screen Shot 2021-01-14 at 2 19 58 PM

@dakoop
Copy link
Contributor

dakoop commented Jan 14, 2021

If I understand the calculation correctly, it should be

calc( (var(--jp-code-line-height) - 1)/2 * var(--jp-code-font-size) / 2 )

since the line height excess is on both sides, too.

@jasongrout
Copy link
Contributor Author

Thanks for looking at this. My thinking was that the line height tells me how much bigger the line is than the font size, so a line height of 1.5 says the line is 50% bigger than the font size. If the font size is 12px, that means the line is 18px, so we need 3px on top and 3px on bottom. (1.5-1)*12px/2 gives us the 3px.

@jasongrout
Copy link
Contributor Author

Using your calculation, we would get 1.5px padding, right? That seems to be too little to me, but there may be something I'm misunderstanding about your calculation, or how line height and font size and padding work. I agree that the padding you are seeing in the inspector seems to be overlapping.

If you change code to your calculation, does it look okay?

@dakoop
Copy link
Contributor

dakoop commented Jan 14, 2021

Hmm, your logic makes sense--I can't justify the extra division, and your understanding of line-height and font-size matches what I've read. What I see is a line-height of 17px, and a font-size of 13px, but the span shows a height of 20px before I halve it and 17px after. This is on a retina display so perhaps that has something to do with it?

@dakoop
Copy link
Contributor

dakoop commented Jan 15, 2021

Ok, it looks like the padding is added to the height of the content-area which is not well-defined for a glyph. Basically, line-height - font-size does not equal the extra padding around the content area. See this. It looks like on the Mac (both Safari and Chrome), the monospace font produces content areas of height 15px which is why my 1px works (versus the 2px computed). There may be ways to make a more informed calculation if you know the font metrics but that seems brittle.

@jasongrout
Copy link
Contributor Author

On the other issue, Max suggested using a colored border instead of padding to extend the coloring. Perhaps a border would be more robust.

@dakoop
Copy link
Contributor

dakoop commented Jan 15, 2021

Borders produce the same overlap issue as padding in my tests.

@github-actions github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Jul 15, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design System CSS pkg:rendermime status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:CSS For general CSS related issues and pecadilloes
Projects
None yet
3 participants