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

Update icon fonts to use svgs #3560

Closed
MMasey opened this issue Nov 8, 2018 · 11 comments · Fixed by #5606
Closed

Update icon fonts to use svgs #3560

MMasey opened this issue Nov 8, 2018 · 11 comments · Fixed by #5606

Comments

@MMasey
Copy link
Contributor

MMasey commented Nov 8, 2018

I'm not sure if this is a feature of a bug.

Icon fonts have a fundamental accessibility problem in that if a person using a tool to change all fonts on a site to be more readable, in the case of those with server dyslexia, all the icon fonts will be replaced with a missing glyph character. This can break the UI, especially if it's dependent on icons.

For example
image

Obviously this example is a v7 site, but i think the issue will apply for v8, as such this issue only applie for v8.

Github went through a similar transition and detailed there approach at https://blog.github.com/2016-02-22-delivering-octicons-with-svg/. I think a similar approach could work for Umbraco, and it could potentially make icons easier to customise.

@nul800sebastiaan
Copy link
Member

@MMasey Thanks Mike! Can you update this issue with the problems you've encountered when you were working on this at the UK hackathon. Might not be worth the trouble to convert it over for now, but I can't remember exactly what the stumbling blocks were?

@nul800sebastiaan nul800sebastiaan added state/needs-more-info We don't have enough information to give a good reply type/feature labels Nov 20, 2018
@MMasey MMasey changed the title Update icon fonts to user svgs Update icon fonts to use svgs Nov 21, 2018
@MMasey
Copy link
Contributor Author

MMasey commented Nov 21, 2018

Sure, no problem. Most of the problems come around the fact that the use of icon fonts is so heavily embedded into the backoffice, although that's more of a time problem than a complexity one.

One on the other problems was that my approach is to use inline SVGs. There would be a folder, potential in the umbraco folder that could contain each of the individual SVG icon files. In order to get this inline, it would require reading the file and printing out the raw SVG data into the html, which is obviously a security issue. Now it is possible to do some validation on an SVG file to ensure that it contains SVG code and not something malicious, although i don't know how it works... yet. The following sites do some form of validation so i know it's possible

https://validator.nu/
https://jakearchibald.github.io/svgomg/

They don't prevent you inserting script tags into the svg for example, but from my research so far, scripts inside SVGs don't get fired.

It think we also mentioned that if they're inside the protected umbraco folder, it should be a bit easier to keep them secure. The reason for this approach is that it potentially makes it really easy for people to add their own icons. I have a feeling this problem may have already been solved for other parts of the backoffice system too. It may also only be a security issue if a hacker gets access to the server itself, which if it's that's the case, there would be far worse problems to deal with.

I'm was also thinking about having an extension method that you can use to say "look at this folder and get the SVGs for the backoffice" which then puts the onus of security on the person making use of the extension.

We could keep the font icon logic there too if people still want to really use it to.

I think overall it is a fair amount of work, but for the accessibility gains and the drastically improved ease of adding your own icons, I think it's worth it. Just imagine it, you could completely customise the icons in the backoffice so that they are completely on brand with a clients site by simple dropping in your own SVG icons into the icon folder, something that is a pain at the moment with icon fonts. It's an improvement from an accessibility, developer and content editor point of view so everyone wins 😄

@nul800sebastiaan nul800sebastiaan added state/hq-discussion-ux and removed state/needs-more-info We don't have enough information to give a good reply labels Nov 26, 2018
@skttl
Copy link
Contributor

skttl commented Jan 8, 2019

Another con for icon fonts, is that if you want to add your own icons to the backoffice, you have to generate (and load) a whole new font face.

Or you could do it the hacky way, like I do in https://our.umbraco.com/packages/backoffice-extensions/custico-custom-icons-for-umbraco/, and add icons as background-images, for the .icon- placeholders. The downside of this, is that you can't manipulate them with css afterwards. So the sweet colorpicker from 7.11 is useless on these icons.

Regarding security, I'm pretty sure that script tags in an inline svg is fired. There is a PHP library for sanitizing SVGs (https://github.com/darylldoyle/svg-sanitizer), maybe that could be ported?

@MMasey
Copy link
Contributor Author

MMasey commented Jan 25, 2019

I've found a package that can do HTML Sanitizing that can have additional tags and attributes added to the list of allowed data so that it can work with SVGs too. I've tested it out and it seems to work ok, and when behind some caching it shouldn't create a burden in terms of performance.

The package is https://github.com/mganss/HtmlSanitizer

I suppose the issue with is it that it has a dependency on AngleSharp, and I know we're trying to reduce dependencies in v8. What are peoples thoughts on using this to handle rendering the svg's out inline?

@MMasey
Copy link
Contributor Author

MMasey commented Feb 5, 2019

Hey @nul800sebastiaan (& @skttl if you're still interested). I currently have a branch that i'm working on for this. As it's a fair amount of work i figured it would be a good idea to share it whilst it's still a WIP so that any question/suggestions/problems etc can be ironed out easier.

If you want to have a look at the commits i've done so far, they can be found at https://github.com/MMasey/Umbraco-CMS/tree/temp8-backoffice-icon-system-update

I'm merging in the latest version of temp8 when it seems appropriate to hopefully reduce the number of potential merge conflicts. As part of this work i'm also fixing a few issues that were occurring with the icon colour picker, but i noticed that someone else has also submitted a PR for these. Due to the large change, my updates take a different approach to their fix, so i'm choosing my changes over theirs for the sake of any merge conflicts. (currently there has only been one)

Thanks

@skttl
Copy link
Contributor

skttl commented Feb 6, 2019

Sweet, I'll give it a spin :)

@MMasey
Copy link
Contributor Author

MMasey commented Feb 18, 2019

Ok, i've been able top make a fair amount of progress with this and so far I have managed to replace every icon font icon with the new umb-icon directive.

// The most basic `umb-icon` directive looks like this
<umb-icon icon-name="icon-profile"></umb-icon>


// You can pass any other attribute through to it as you would any other directive, such as...
<umb-icon icon-name="icon-profile" class="icon icon-profile large" ng-show="model.show"></umb-icon>

All of the individual icon svgs now live under src\Umbraco.Web.UI.Client\src\assets\icons. When the frontend build process is run these get copied over to the src\Umbraco.Web.UI\Umbraco\assets folder, in the same way the other frontend source files do.

These are 2 API endpoints that can be used to get the SVG markup. The controller for these can be found at v8\src\Umbraco.Web\Editors\IconController.cs

GetIcon

returns a single IconModel

e.g. - http://umbraco.v8/umbraco/backoffice/UmbracoApi/Icon/GetIcon?iconName=icon-navigation-right

returns
{
    "Name":"icon-navigation-right",
    "SvgString":"<svg xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 512 512\"><path d=\"M360.124 255.513L148.535 422.442l.002-333.862z\"/></svg>"
}

GetAllIcons

returns List (all icons found in the icons folder, used for the icon picker)

e.g. - http://umbraco.v8/umbraco/backoffice/UmbracoApi/Icon/GetAllIcons

returns
[
{
    "Name":"icon-navigation-right",
    "SvgString":"<svg xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 512 512\"><path d=\"M360.124 255.513L148.535 422.442l.002-333.862z\"/></svg>"
},
{
    "Name":"icon-navigation-left",
    "SvgString":"<svg xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 512 512\"><path d=\"M360.124 255.513L148.535 422.442l.002-333.862z\"/></svg>"
}
,...]

The beauty of this approach is that if you want to add more (non-core) icons, you can add them to the src\Umbraco.Web.UI\Umbraco\assets folder and you will have access to them. The means for people creating apps for Umbraco will be able to easily add their own icons. The same goes for those wanting to do other personalisations to the backoffice.

In order to see the difference i highly recommend installing the Font Changer with Google Web Fonts Chrome extension and view the icon-font version to get an idea of the problem this solves.

I still haven't added the sanitizer as i'd like some guidance on this.

If you have any questions please let me know. I'd love to get some feedback on this, but i don't think it's ready for a PR yet (although i'm happy to create one if it will be beneficial)

My branch can be found at https://github.com/MMasey/Umbraco-CMS/tree/temp8-backoffice-icon-system-update

@MMasey
Copy link
Contributor Author

MMasey commented Apr 17, 2019

I've put together a draft PR that can be found at #5293 🙂

@MMasey
Copy link
Contributor Author

MMasey commented Apr 17, 2019

I've found a package that can do HTML Sanitizing that can have additional tags and attributes added to the list of allowed data so that it can work with SVGs too. I've tested it out and it seems to work ok, and when behind some caching it shouldn't create a burden in terms of performance.

The package is https://github.com/mganss/HtmlSanitizer

I suppose the issue with is it that it has a dependency on AngleSharp, and I know we're trying to reduce dependencies in v8. What are peoples thoughts on using this to handle rendering the svg's out inline?

In regards to the SVG sanitization, I have a separate project in which I have used the suggested package, add added a bunch of custom config to make it work with SVG's. So far it has worked like a charm.

@MMasey
Copy link
Contributor Author

MMasey commented Jun 6, 2019

PR #5606 (finally)

@nul800sebastiaan
Copy link
Member

I see that I've labeled the PR, but forgot there's an issue for it, I'll move the labels from the PR to here (this goes into the release notes) and close the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants