t/142: Button tooltip should not look blurry on ldpi screens

[oleq]

Suggested merge commit message (convention)

Fix: Button tooltip should not look blurry on ldpi screens. Closes ckeditor/ckeditor5#5302. Closes ckeditor/ckeditor5#5294.

BREAKING CHANGE: The ck-tooltip__main, ck-tooltip__arrow and ck-tooltip__elements mixins are no longer available. CSS classes should be used instead.

---

Additional information

• Requires t/103: Refactored tooltip styles to allow tooltips which are no longer pseudo-elements ckeditor5-theme-lark#104.

[Mgsy]

It seems that this fix solves the problem with blurry tooltips. Text on labels is sharp and you can easily read it.

Also, I didn't notice any issues caused by this fix.

[oskarwrobel]

My only doubt is that tooltip is not so easy to use. I used to use tooltips that can be easily attached to any element by simple API or [data-*] attributes.

[oskarwrobel]

I'm wondering if tooltip could be a mixin? This mixin could be mixed with any View and then tooltip options could be set by [data-*] attributes. Does it make any sense?

[oleq]

I'm wondering if tooltip could be a mixin? This mixin could be mixed with any View and then tooltip options could be set by [data-*] attributes. Does it make any sense?

Some example?

[oskarwrobel]

The idea with [data-*] was stupid. But still...

mix( ButtonView, TooltipMixin );

const button = new ButtonView();

button.tooltip = 'tooltip text';
button.tooltipPosition = 's';

Is it possible?

[oleq]

I don't think so. You need to either

• extend ButtonView#template to inject the toolip DOM elements
• create the TooltipView and put it in an already rendered ButtonView#element

I think neither is going to happen with mix(). Inheritance yes, but not sure how mix() could help.

[Reinmar]

The current code looks ok to me. It's not hard to use the tooltip. You just need to create it in your view, add it as a child of it and set its 2 properties. The button view class exposes few things (the position for instance). But if you don't have such a requirement then adding a tooltip should be 3-4 lines of code. Am I right?

[oleq]

What we can do, however, is a helper like this:

enableViewTooltip( view, tooltipTextAttribute, tooltipPositionAttribute ) {
	const tooltipView = new TooltipView();

	tooltipView.bind( 'text' ).to( view, tooltipTextAttribute );
	tooltipView.bind( 'position' ).to( view, tooltipPositionAttribute );
	view.element.appendChild( tooltipView.element );

	// Make sure the tooltip will be destroyed along with the view.
	view.addChildren( tooltipView );

	return tooltipView;
}

I'm not sure it would be very useful though as it hard-codes the hierarchy in DOM, etc..

[Reinmar]

It could be a static method of TooltipView. E.g. TooltipView.enableIn( view ). I'd make the attribute names standardised so you don't have to pass them. If you use different ones – create the view manually. In fact, without binding, if values of these two properties could be passed directly to `TooltipView(), that would be 3 lines of code:

const tooltip = new TooltipView( 'title', 'position' );
this.element.appendChild( tooltip.element );
this.addChildren( tooltip );

[oskarwrobel]

In fact, without binding, if values of these two properties could be passed directly to `TooltipView(), that would be 3 lines of code

That's true. It's not so complex :)

Code looks fine, tooltips works 👌. I'm merging it.

[oskarwrobel]

Wait :)

if values of these two properties could be passed directly

This is missing :)

[oskarwrobel]

Anyway, for me current code is fine.

[oskarwrobel]

It's not enough to implement tooltip in JS. It needs to be handled in CSS as well.

.ck-button:hover {
	@include ck-tooltip_visible();
}