[Core/Menus] pt-text-overflow-ellipsis cuts off descenders for menu items

[will-stone]

Bug report

• Package version(s): 2.0.0-rc.2
• Browser and OS versions: Chrome 64, macOS Sierra 10.12.6

Steps to reproduce

1. Create a MenuItem with text that has descenders e.g. "g", "y" etc.

Actual behavior

Expected behavior

All text is visible.

This looks to be related to #1689

[adidahiya]

I think this is a regression in 2.0, or at least in the 2.0 documentation. Descenders are not cut off in 1.x menu items: http://blueprintjs.com/docs/v1/#core/components/menu.menu-item

[giladgray]

yikes, definitely due to the flex stuff. should be easy to resolve. i'm on it.

[reiv]

Can we reopen this? It doesn't seem fixed, at least not for all browser/font combinations. Here
are some screenshots from testing on Chrome and Firefox (Windows only, sorry):

Chrome Version 67.0.3372.0 (Official Build) canary (64-bit)
Firefox 61.0a1 (2018-03-16) (64-bit)

The culprit is definitely line-height (currently 16px for MenuItems), which doesn't leave enough room for some fonts' descenders, and overflow: hidden from pt-text-overflow-ellipsis prevents overflow; zoom level is also a factor here, and apparently browsers have different interpretations as well. The current "one-size-fits-all" approach is unfortunately not good enough 😞.

Here's a JSFiddle for testing -- it uses core@^2.0.0-rc.3 but the screenshots were taken using the latest develop.

@giladgray @adidahiya

[giladgray]

@reiv sure, it is reopened. think you could submit a fix?

[reiv]

@giladgray I'll see what I can do. I was hoping the fix would be as easy as replacing overflow: hidden with overflow-x: hidden, but that doesn't work quite the way you'd expect it to.

[giladgray]

in my experience, one should never use overflow-x/y as they don't do what you think/hope they do

[reiv]

Would we run into trouble elsewhere if we removed the line-height rule from pt-menu-item? Reading the SCSS gives the impression that line-height was only being used for centering the icon, but since pt-menu-item is a flex container, we can use align-self: center on the icon to line things up instead.

blueprint/packages/core/src/components/menu/_common.scss

Lines 22 to 28 in 7166b66

	@mixin menu-item($disabled-selector: ".pt-disabled", $hover-selector: ":hover") {
	@include pt-flex-container(row, $menu-item-padding);
	align-items: center;
	border-radius: $menu-item-border-radius;
	padding: $menu-item-padding;
	text-decoration: none;
	line-height: $pt-icon-size-standard;

Updated JSFiddle