i/5806: Rounded corners should work for dropdown panel children in all positions

[oleq]

Suggested merge commit message (convention)

Fix: Rounded corners should work for dropdown panel children in all panel positions. Closes ckeditor/ckeditor5#5806.

---

[dkonopka]

I've found some minor issue, but this is related to the dropdown panel I guess and we can make it as followup?

[Reinmar]

@oleq @panr could you close this? :D One way or the other.

[oleq]

I've found some minor issue, but this is related to the dropdown panel I guess and we can make it as followup?

This cannot be fixed. The roundness of the panel is OK because the panel assumes the content will be horizontal and wider than the dropdown button.

It's the toolbar that is vertical (it has a class) and the dropdown has no way to learn about it. We'd need to re-write some logic in JS to give the panel some class if the toolbar child is vertical but I thing this is way too much hassle.

[oleq]

Anyway, let's give this PR another try. @panr?

[panr]

👇🏻More like discussion than requesting changes right now.

[panr]

I don't like it... Using this mixin here, repeatedly, makes the code too long and hard to read IMHO.
I think we should create a similar mixin but for reseting radiuses. Then we can mix it with ck-rounded-corners but also use it here to reset those and apply radius only for the one corner. WDYT?

[oleq]

This is what the mixin looks like https://github.com/ckeditor/ckeditor5-theme-lark/blob/0115e9440ce152135164ba4e4b6ad831b56ab6fd/theme/mixins/_rounded.css#L11-L20

All it does it border-radius: var(--ck-border-radius); and then everything from the brackets. It works as an exclusion.

A mixin for resetting radiuses would... look exactly the same as @mixin ck-rounded-corners I guess. So I'm not sure we would gain that much code. postcss-mixins does not support conditionals so there's no way to specify which radiuses should be reset.

[panr]

Yeah, I know how the mixin looks like, checked it before ;-) So basically:

@mixin ck-rounded-corners {
	border-top-right-radius: 0;
	border-bottom-right-radius: 0;
	border-bottom-left-radius: 0;
}

does the same thing as:

border-radius: 0;
border-top-left-radius: var(--ck-border-radius);

right?

The latter is much simpler to me, so my suggestion is to apply the code above or introduce:

@mixin ck-reset-corners {
    border-top-left-radius: var(--ck-border-radius);
}

[oleq]

It makes sense but honestly, I'm confused by the naming. ck-rounded-corners enables rounded corners then you can opt-out from some of them. ck-reset-corners also adds rounded corners but it does not make sense standalone without @mixin-content.

Maybe we should investigate and see if there's a chance for us to write mixins in JS. We could have a better rounded-corners mixin then:

/* Enable all */
@mixin ck-rounded-corners;

/* Enable bottom-right and top-left only */
@mixin ck-rounded-corners bottom-right top-left;

/* Do something custom (it only adds selectors) */
@mixin ck-rounded-corners {
    border-top-left-radius: 2cm;
}

I'm not sure how this could work with our ultra-modular framework. That's probably a follow-up.

[panr]

I get your point here. So to wrap it all up, you still prefer this:

@mixin ck-rounded-corners {
	border-top-right-radius: 0;
	border-bottom-right-radius: 0;
	border-bottom-left-radius: 0;
}

over this:

border-radius: 0;
border-top-left-radius: var(--ck-border-radius);

?

[panr]

Also #254 (comment) I think there is a chance to handle this. We can set overflow: hidden to the element and then set the right radius to it. Yes I know that this will trim the visibility of the tooltip, but there is a trick to set this right.

- parent/container (position: relative)
  - element (position: static; overflow: hidden)
    - tooltip (position: absolute)

[oleq]

Also #254 (comment) I think there is a chance to handle this. We can set overflow: hidden to the element and then set the right radius to it. Yes I know that this will trim the visibility of the tooltip, but there is a trick to set this right.

If you set the overflow, this will not resolve the issue b/c the panel has the rounded upper-right corner anyway, which is right and wrong at the same time.

Right, because this is a south-west panel, like the heading panel. It is not aware (on the JS level) of the fact it is so narrow its upper-right corner is aligned with the button that opens it.

Wrong, because it looks odd. And there's no way to change it ATM without refactoring in JS that would probably add another class.

---

Long story short this is what it corresponds to

and even if it was fixed

this will still look weird after all

[panr]

#254 (comment) Gotcha!

But I'm still thinking about this trick... Maybe it will help us reduce some redundant radius resets🤔