Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#13119 closed Task (fixed)

Improve skins' CSS resets

Reported by: Piotrek Koszuliński Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.5.0
Component: General Version:
Keywords: Cc:

Description

The new samples proved that the current CSS reset rules are insufficient. For instance - margin of sourcearea's textarea, underline and color of links.

Attachments (2)

basic_sample_source_code_styling.png (19.2 KB) - added by Wiktor Walc 10 years ago.
Screen Shot 2015-04-21 at 14.51.26.png (157.0 KB) - added by Piotrek Koszuliński 10 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 10 years ago by Piotrek Koszuliński

Status: newconfirmed

comment:2 Changed 10 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

comment:3 Changed 10 years ago by Piotrek Koszuliński

I've been thinking about this and I've got few observations:

  1. We can't obviously create a bulletproof skin. If someone decides to style #content div { float: left; border: solid 5px red } it's his problem.
  2. .cke_reset_all * {} has lower specificity than .mycontent a {}... That was really surprising for me and it's sad because it makes our reset rules less significant than usual .mycontent sometag {} styles.
  3. It's pretty common to style some very specific tags (by specific I mean for instance links, but not divs or spans) with rules of specificity 1 - e.g. all links on the page, or all inputs. With this we deal pretty well thanks to our reset.
  4. But it's even more common to style specific tags in some container (specified by class or id). With the id we can't do anything, but with the class we can and I saw on many occasions that our certain styles get overridden.

In my opinion links cause the biggest problem. We don't have right not a rule with specificty 11+ which clean them. Another tag which I saw touched is the sourceare and that's because it's not inside any cke_reset_all element.

I pushed branch:t/13119 with a manual test in which I styled content accordingly to the above rules - that we can't be bulletproof, but we can do better with specific elements rules (I chose only link and textarea) which have specificity 11 and set some reasonable (popular) styles.

I also made necessary changes to our reset.css (added .cke_reset_all a, .cke_reset_all textarea rules) and tuned related rules so they have specificity of 11+ too.

WDYT about the above? Please remember that the patch is not meant to solve all problems, but simply improve the situation for most common scenarios.

PS. This small research was pretty interesting and was a good lesson that we could use when working on CKEditor 5 or any new skin that we'll build. It proves that CSS reset should be based on rules of specificity 11+ for as many elements that we use in our UI as possible. It can't be done for moono or kama now, because if I added .cke_reset_all span for instance it would make all other rules based on classes only (specificty = 10) ignored.

Last edited 10 years ago by Piotrek Koszuliński (previous) (diff)

comment:4 Changed 10 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.5.0 Beta

We changed samples framework styling a little bit, so the issues that we had are gone. I'm removing milestone, but I still wait for your feedback.

comment:5 Changed 10 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.5.0

The issue is back in SDK.

Changed 10 years ago by Wiktor Walc

comment:6 Changed 10 years ago by Wiktor Walc

In basic sample there is some empty space above the source code.

Changed 10 years ago by Piotrek Koszuliński

comment:7 Changed 10 years ago by Piotrek Koszuliński

WFM (see http://dev.ckeditor.com/attachment/ticket/13119/Screen%20Shot%202015-04-21%20at%2014.51.26.png - I can't attach it in this comment because Trac can't deal with spaces :D)

comment:8 Changed 10 years ago by Piotrek Koszuliński

Uh, I confirm this issue on Firefox.

comment:9 Changed 10 years ago by Piotrek Koszuliński

And the problem is that it has the default display:inline, while it should have display:block to be independent on line heights and font sizes.

comment:10 Changed 10 years ago by Piotrek Koszuliński

Status: assignedreview

I pushed more fixes to branch:t/13119. Both skins are improved.

comment:11 Changed 10 years ago by Olek Nowodziński

Status: reviewreview_passed

Even if something's wrong with the changes, we'll get a chance to spot it in the course of development. So better merge it sooner than later.

comment:12 Changed 10 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

Merged into major in git:2e0eee9.

comment:13 Changed 10 years ago by Piotrek Koszuliński

Type: BugTask
Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy