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

Editable needs padding #5438

Closed
Reinmar opened this issue Dec 11, 2017 · 6 comments · Fixed by ckeditor/ckeditor5-theme-lark#117
Closed

Editable needs padding #5438

Reinmar opened this issue Dec 11, 2017 · 6 comments · Fixed by ckeditor/ckeditor5-theme-lark#117
Assignees
Labels
package:ui type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Dec 11, 2017

I think I reported it already but I can't see that ticket now.

The problem is that editable has no padding, so when you style your blocks to have the only bottom margin (which is common, e.g. GitHub does it), then that content looks terribly in the editor.

image

OTOH, if we'd simply add padding at the top then we'd make the content look bad with user agent styles which set block's top and bottom margins. So, to workaround that we'd also need to add :first-child and :last-child styles. I'm not sure this is worth it, but I had to fix editable's styling in at least 2 integrations already (Umberto and GH RTE), so this is a quite common problem.

@oleq
Copy link
Member

oleq commented Dec 11, 2017

Some possible solutions, each has some drawbacks:

A

  • .ck-editor__editable_inline with top/bottom padding
  • :first-child { margin-top: 0 }
  • :last-child { margin-bottom: 0 }

Pros:

  • Simple

Cons:

  • Breaks when the fake selection container appears at the end of the editable. The content jumps when the (image) widget gets selected.
<img/>
<p>foo</p> /* <- this one has margin-bottom: 0 */
[<img/>]
<p>foo</p> /* <- this one should have margin-bottom: 0 */
<div>fake sel container</div> /* <- but this container gets it instead */

B

  • .ck-editor__editable_inline with top/bottom padding
  • nothing else

Pros:

  • Bulletproof

Cons:

  • Doesn't look that great because editable's padding duplicates first/last child's margins
    image

C

  • .ck-editor__editable_inline without top/bottom padding
  • :first-child { margin-top: non-zero }
  • :last-child { margin-bottom: non-zero }

Pros:

  • Simple

Cons:

  • Will also break when the fake selection container kicks in.

@Reinmar
Copy link
Member Author

Reinmar commented Dec 11, 2017

What about C but with first-child only? We know that fake-sel-con is the last child so it's better to not touch this. OTOH, does absolutely positioned element's margin count?

@oleq
Copy link
Member

oleq commented Dec 11, 2017

What about C but with first-child only?

That makes some sense. We could use A and leave last-child untouched too. In other words: any solution which does not touch last-child.

OTOH, does absolutely positioned element's margin count?

No, it doesn't. Anyway, by

Will also break when the fake selection container kicks in.

I meant that there could (?) be some last-child element in the content without margin-bottom and fake–sel div would "steal" this :last-child { margin-bottom: non-zero } style. WDYT?

@oleq
Copy link
Member

oleq commented Dec 11, 2017

Or... we could force the engine to apply some CSS class to the last non–fake–selection element in the content 😅

@Reinmar
Copy link
Member Author

Reinmar commented Dec 11, 2017

Or... we could force the engine to apply some CSS class to the last non–fake–selection element in the content 😅

This goes too far :P

That makes some sense. We could use A and leave last-child untouched too. In other words: any solution which does not touch last-child.

So maybe let's just not overcomplicate this. Some minimal first-child's top margin solves this issue and is easy enough to understand.

@oleq oleq self-assigned this Dec 12, 2017
@oleq
Copy link
Member

oleq commented Dec 12, 2017

Created PR for this issue ckeditor/ckeditor5-theme-lark#117.

Reinmar referenced this issue in ckeditor/ckeditor5-theme-lark Feb 2, 2018
Fix: First child of editable should always have a top margin to make sure the content is separated. Closes #116. Closes ckeditor/ckeditor5-ui#351.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-ui Oct 9, 2019
@mlewand mlewand added this to the iteration 14 milestone Oct 9, 2019
@mlewand mlewand added type:bug This issue reports a buggy (incorrect) behavior. package:ui labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:ui type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants