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.

Sign up for GitHub

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

Jump to bottom

IconView should preserve (some) attributes of the <svg> tag #12597

Closed
oleq opened this issue Oct 7, 2022 · 0 comments · Fixed by #12631
Closed

IconView should preserve (some) attributes of the <svg> tag #12597

oleq opened this issue Oct 7, 2022 · 0 comments · Fixed by #12631
Assignees
@oleq
Labels
package:ui squad:features Issue to be handled by the Features team. type:bug This issue reports a buggy (incorrect) behavior.
Milestone
iteration 58 (v35...

Comments

@oleq
Copy link
Member

oleq commented Oct 7, 2022 •
edited by mlewand
Loading

📝 Provide detailed reproduction steps (if any)

If you create an icon with 

<svg width="45" height="45" viewBox="0 0 45 45" fill="none" xmlns="http://www.w3.org/2000/svg">

the fill="none" will be ignored because IconView creates the <svg> element from scratch. Instead, we should preserve these attributes because they affect rendering. Without this attribute the icon can look broken.

✔️ Expected result

fill="none" stays

❌ Actual result

fill="none" is gone

❓ Possible solution

  • To make it clear, we need to pass only certain attributes from predefined white list.
  • We're concerned only about attributes that are added to the root svg element.

The fix should land around https://github.com/ckeditor/ckeditor5/blob/8254ae2dc7e0cfb23d15dc040a2c94f32ac0d3c5/packages/ckeditor5-ui/src/icon/iconview.js#L98-L112.

we should copy all attributes except width, height (handled by our viewbox), viewbox (already copied), xmlns (handled by createElementNS).

If you'd like to see this fixed sooner, add a 👍 reaction to this post.
@oleq oleq added type:bug This issue reports a buggy (incorrect) behavior. package:ui labels Oct 7, 2022
@oleq oleq changed the title IconView should use attributes of the <svg> tag IconView should preserve (some) attributes of the <svg> tag Oct 7, 2022
@oleq oleq added the squad:features Issue to be handled by the Features team. label Oct 7, 2022
@oleq oleq self-assigned this Oct 7, 2022
@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. and removed status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. labels Oct 7, 2022
@mlewand mlewand mentioned this issue Oct 10, 2022
Closed
@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Oct 11, 2022
@oleq oleq mentioned this issue Oct 11, 2022
Merged
oleq added a commit that referenced this issue Oct 14, 2022
@oleq
Merge pull request #12631 from ckeditor/ck/12597-icon-view-allow-unmo…
57100fa 
…dified-icons

Fix (ui): Improvements to the `IconView` component to allow rich colorful icons. Closes #12597. Closes #12599.

Preserved presentational attributes from the icon source on the `<svg>` element (#12597). 
Made it possible to opt out from icon color inheritance (#12599). 
Excluded icon internals from CSS reset (#12599).
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Oct 14, 2022
@CKEditorBot CKEditorBot added this to the iteration 58 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@oleq oleq

Labels
package:ui squad:features Issue to be handled by the Features team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Milestone
iteration 58 (v35.3.0)
Development

Successfully merging a pull request may close this issue.

Improvements to the IconView to allow rich colorful icons ckeditor/ckeditor5
2 participants
@oleq @CKEditorBot