Don't set fill for icons that set declare it

[michaeldjeffrey]

Only override the fill for elements that don't declare one.
Closes ckeditor/ckeditor5#3420

[michaeldjeffrey]

Closed because I'm a doofus and named the branch incorrectly

[michaeldjeffrey]

Wait no, I'm extra dumb. 207 is the PR ticket. I didn't sleep well. sigh

[oleq]

Before we can merge this patch, we need to do 2 things:

• Clean-up existing icons. There are nearly 50 uses of fill="..." attribute in SVG icons across the project. If we merged this PR, which narrows the feature to fill–less nodes only, the dynamic icon colors wouldn't work anymore for all those icons. This is a regression we must avoid.
• We must verify what multi–color icons look like after this fix. And I'm speaking mainly about highlight icons which are configurable.
  

[michaeldjeffrey]

I made a pen with most (I think all) icons from the ckeditor repos https://codepen.io/michaeldjeffrey/pen/YRGWLZ

Regarding icons like Highlight. The colors are set by setting the style attribute to use the css variable. If a fill attribute is also provided on an icon like that it will be ignored.

Something I noticed in making that codepen, some of the icons like copyformatting/cursors/cursor-disabled.svg are supposed to have colors in them, but the fill is overridden.

Other icons of note. The icons/table.svg declares its fill on the path, so it's using the new rule it's color would be defined unless overridden in css. But the companion icons to that icons/table-column.svg and friends declare a fill on the <g> so their fill is ignored anyways. It seems to truly respect an fill attribute, there would have to be a rule that doesn't set the fill color of anything that has a parent that sets a fill color.

I guess a couple options would be:

• Removing any fill declaration that appears to be there as a text color.
• Move fill attributes on <g> to each item inside.

What would you like to see from me to move this forward?

[oleq]

To start off, I noticed that you mixed CKEditor 4 and CKEditor 5 icons. They are two different projects and ckeditor-dev/** is a main v4 repository folder. There is no copyformatting feature in v5.

Just to let you know, we already have a manual test that agregates all icons in the project:

Having configured the development environment, you can run it using npm run manual -- --files theme-lark. You can use it for testing and development, which feels like a more complete tool when compared to your pen.

---

Back to the issue, I think we could get rid of the fill="" attribute across the project if we decided to merge this PR. OTOH, that would make them useless if someone used them in some other way than what we do in the project (because they would become transparent). Should we care about that? (cc @Reinmar @dkonopka).

[Reinmar]

that would make them useless if someone used them in some other way than what we do in the project (because they would become transparent). Should we care about that?

If someone used our SVG files in other places, not styled with our .ck-icon class, yes? I think it's ok – our way of using those icons (with a color applied via CSS) is the way to use them. They don't have fill cause they are colorable. Sounds reasonable to me.

[dkonopka]

(because they would become transparent)

I was suprised, but it isn't actually true. Even clean .html file will fill by default svg with #000000, same with software like Sketch or Adobe Illustrator.

To sum up I'm 👍 for removing it. By the way we could decide if we are using width/height or viewBox property for our <svg> files.

[oleq]

And whatever convention we agree upon, let's write it down in https://ckeditor.com/docs/ckeditor5/latest/framework/guides/contributing/development-environment.html#svg-icons.

[oleq]

I pushed changes to the SVG icons in various repos. The changes are as follows:

• removed fill attrs,
• removed width and height, leaving just the viewBox,
• removed useless <g>,
• removed useless fill-rule attrs.

You can check them by using mgit update in a root branch https://github.com/ckeditor/ckeditor5/compare/t/ckeditor5-theme-lark/206?expand=1 and test with the patch introduced in this PR.

Note: Since I automated my work with regexps and svgo, the changes must be thoroughly reviewed and verified in all browsers.

cc @dkonopka

[oleq]

Other PRs in a human-readable format:

• root (constellation, new npm task and docs)
• ckeditor5-alignment
• ckeditor5-basic-styles
• ckeditor5-core
• ckeditor5-font
• ckeditor5-highlight
• ckeditor5-heading
• ckeditor5-image
• ckeditor5-link
• ckeditor5-list
• ckeditor5-paragraph
• ckeditor5-media-embed
• ckeditor5-table
• ckeditor5-undo
• ckeditor5-ui
• ckeditor5-widget

Have fun, @dkonopka 😛

[oleq]

BTW, this change saves us ~2.5kB of code.

[Reinmar]

How did you clean those svg files? Did you use some tool? Is it documented in the dev env guide?

[oleq]

How did you clean those svg files? Did you use some tool? Is it documented in the dev env guide?

On my TODO.

[Reinmar]

Let's keep this PR open until we have the docs too 😈

[oleq]

@Reinmar Waiting in ckeditor/ckeditor5#1361.

[oleq]

Thanks, @michaeldjeffrey for your contribution! Let us know if everything is OK now.

[michaeldjeffrey]

Thanks for running with this. Much appreciated.