Modify Logo skin object to handle SVGs #3898

X3Technology · 2020-07-06T19:16:36Z

Resolves #3893

DNN Platform/Website/admin/Skins/Logo.ascx.cs

valadas · 2020-07-06T20:32:15Z

For the sanitization part I believe a simple try catch, if there is not svg node found we just log the info and don't show any logo.

As for third party, I think using the File manager class is the way to go? @mitchelsellers

mitchelsellers · 2020-07-06T20:47:47Z

@mathisjay @valadas Yes, I think the FileManager class is the way we will have to go for the download of the file for parsing as PhysicalPath is not always going to be there and not always locally.

I'm going to throw a few more notes in on a review form a performance perspective.

Lastly, in this situation if the provided SVG file is not valid, an empty string is going to be rendered, is this proper? I think we need to give something better for a fallback?

mitchelsellers

@mathisjay FIrst of all, I don't want to be critical here at all, so please do call/email me directly with any concerns that you may have about this. This is just a mission-critical element that impacts every page-load of a site so I want to be sure that we have it right. See the attached.

One additional note/question for @dnnsoftware/approvers, this will also create a bit of a Regression in out of the box functionality with regards to ADA Compliance it seems as most likely we would need to take the existing Alt text and put it into the <title> of the SVG from what I've found. (https://css-tricks.com/accessible-svgs/) I don't typically do this with SVG's so I cannot confirm with actual compliance testing

DNN Platform/Website/admin/Skins/Logo.ascx.cs

mitchelsellers · 2020-07-06T20:55:36Z

DNN Platform/Website/admin/Skins/Logo.ascx.cs

+                                svg = cacheSvg;
+                            }
+
+                            Literal litSvg = new Literal();


We need some sort of fallback at this point if the file couldn't be obtained.

What would you suggest here? Inject the Portal Name as string literal?

I'm not sure on this, but there needs to be something somewhere I think

Also, this still needs a try/catch around it

X3Technology · 2020-07-06T20:57:54Z

I'll look into the FileNamager class, not used that before.
If the .svg is not valid, nothing will be injected into the hyperlink. I guess we could inject the Site Name just so it is not empty. but we aren't doing anything special if they select an invalid .jpg for the logo, it will just show a broken image. I'd say, just keep it simple...

X3Technology · 2020-07-06T23:45:54Z

Made the requested changes. I just rewrote the whole process since the previous code was only expecting an image and not efficient when allowing other types.

The previous code was already using the FileManager class to load the Logo file, so I think we are good there. Please verify.

X3Technology · 2020-07-07T01:01:42Z

I made a few more optimizations.

I tested the current code with the following use cases;

No Logo file is specific -> empty hyperlink (current behavior)
Image type (jpg, png, gif) specified, but not found -> broken image shown (current behaviour) - this could only happen if the file were deleted after being selected
SVG file selected as logo but theme skin object DOES NOT request InjectSVG -> Displays SVG using an tag (current behaviour)
SVG file selected and theme skin object DOES request InjectSVG -> Display SVG using an SVG tag and uses cache for SVG code
Skin object requests InjectSVG but Logo file is not an SVG file type -> hyperlink text shows the PortalName instead.
Skin object request InjctSVG and the file type is .svg but the file contains no tag -> hyperlink text shows the PortalName instead.

I believe this maintains all the current functionality and only enables the new behaviour if the skin object requests InjectSVG.

valadas · 2020-07-07T08:31:12Z

@mathisjay as discussed earlier, i was working on this PR probably at the same time as you and just read your last comment after pushing to your branch... Let me know if you like my changes or if I messed up yours...

@mitchelsellers would you re-review, this should:

Not change the existing behaviours (needs the InjectSvg opt-in)
Will do some validation for an existing svg xml node
Will add the recommended ADA compliance information.

I also fixed the Stylecop warnings on this file.

X3Technology · 2020-07-07T17:23:47Z

Thanks @valadas
I added a new opt-in property LinkCssClass that will put the Css class on the hyperlink instead of the <img> or <svg>

DNN Platform/Website/admin/Skins/Logo.ascx.cs

valadas · 2020-07-07T23:29:22Z

Ok, what do you think now @mitchelsellers ?

bdukes · 2020-07-10T16:09:28Z

I was having a hard time reviewing the changes because of how much of the original logic was adjusted or moved around. I've re-created what y'all did, but arranged it a little differently in my fork 8428629.

Let me know if y'all see any issues in that. When taken altogether, I think it results in a diff that's a lot cleaner.

The only issue I saw in my testing was that since we're caching SVG document, changing the CssClass property doesn't have an immediate effect until you do something to change the cache. Is that just a trade-off we need to make for performance (@mitchelsellers)?

bdukes · 2020-07-10T16:17:42Z

I've pushed a squashed version to ab2a618, which should be easier to review.

bdukes · 2020-07-10T18:37:26Z

Just wanted to make a reference in the PR to @mathisjay's comment on the commit ab2a618#commitcomment-40515132 /cc @valadas

valadas · 2020-07-11T05:32:20Z

It is a bit hard to follow up on commits in a fork for a PR :)

But yeah, IMO the only issue with the changes on your fork is that if the svg has no classes to begin with but we give the skinObject a CssClass, then it would include a space with your changes as in " myclass" (hence why I put it in a list, added the class and then joined them). I don't believe this List is a huge performance hit, this would just be called on first load or cache clear it would not get hammered much...

Resolves dnnsoftware#3893 Co-authored-by: Daniel Valadas <info@danielvaladas.com> Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>

bdukes · 2020-07-13T13:32:55Z

@valadas thanks for the feedback, I've adjusted how we're generated the value of the class attribute:
svgXmlNode.SetAttributeValue("class", string.IsNullOrEmpty(classes) ? this.CssClass : $"{classes} {this.CssClass}");

valadas · 2020-07-13T20:26:20Z

@valadas thanks for the feedback, I've adjusted how we're generated the value of the class attribute:
svgXmlNode.SetAttributeValue("class", string.IsNullOrEmpty(classes) ? this.CssClass : $"{classes} {this.CssClass}");

Oh, yeah, love it.

bdukes · 2020-07-20T15:00:28Z

@valadas @mitchelsellers can we get another 👍 and get this merged?

mitchelsellers · 2020-07-20T15:16:31Z

Second review is there but having issues merging if someone else can

bdukes · 2020-07-20T15:17:43Z

@valadas we need you to clear your "request changes" review before we can merge

valadas · 2020-07-23T01:02:39Z

Sorry guys, done and done

valadas requested changes Jul 6, 2020

View reviewed changes

DNN Platform/Website/admin/Skins/Logo.ascx.cs Outdated Show resolved Hide resolved

mitchelsellers requested changes Jul 6, 2020

View reviewed changes

DNN Platform/Website/admin/Skins/Logo.ascx.cs Outdated Show resolved Hide resolved

DNN Platform/Website/admin/Skins/Logo.ascx.cs Outdated Show resolved Hide resolved

mitchelsellers requested changes Jul 6, 2020

View reviewed changes

valadas requested changes Jul 7, 2020

View reviewed changes

DNN Platform/Website/admin/Skins/Logo.ascx.cs Outdated Show resolved Hide resolved

bdukes added this to the 9.7.0 milestone Jul 7, 2020

bdukes added the Type: Enhancement label Jul 7, 2020

Add InjectSvg property to Logo theme object

4e90e24

Resolves dnnsoftware#3893 Co-authored-by: Daniel Valadas <info@danielvaladas.com> Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>

bdukes force-pushed the issue-3893 branch from dd33b94 to 4e90e24 Compare July 13, 2020 13:26

bdukes approved these changes Jul 13, 2020

View reviewed changes

bdukes requested review from valadas and mitchelsellers July 13, 2020 13:29

mitchelsellers approved these changes Jul 20, 2020

View reviewed changes

valadas approved these changes Jul 23, 2020

View reviewed changes

valadas merged commit 0d3b931 into dnnsoftware:develop Jul 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify Logo skin object to handle SVGs #3898

Modify Logo skin object to handle SVGs #3898

X3Technology commented Jul 6, 2020

valadas commented Jul 6, 2020

mitchelsellers commented Jul 6, 2020

mitchelsellers left a comment •

edited

Loading

mitchelsellers Jul 6, 2020

X3Technology Jul 6, 2020

mitchelsellers Jul 6, 2020

mitchelsellers Jul 6, 2020

X3Technology commented Jul 6, 2020

X3Technology commented Jul 6, 2020

X3Technology commented Jul 7, 2020

valadas commented Jul 7, 2020

X3Technology commented Jul 7, 2020

valadas commented Jul 7, 2020

bdukes commented Jul 10, 2020

bdukes commented Jul 10, 2020

bdukes commented Jul 10, 2020

valadas commented Jul 11, 2020

bdukes commented Jul 13, 2020

valadas commented Jul 13, 2020

bdukes commented Jul 20, 2020

mitchelsellers commented Jul 20, 2020

bdukes commented Jul 20, 2020

valadas commented Jul 23, 2020

Modify Logo skin object to handle SVGs #3898

Modify Logo skin object to handle SVGs #3898

Conversation

X3Technology commented Jul 6, 2020

valadas commented Jul 6, 2020

mitchelsellers commented Jul 6, 2020

mitchelsellers left a comment • edited Loading

Choose a reason for hiding this comment

mitchelsellers Jul 6, 2020

Choose a reason for hiding this comment

X3Technology Jul 6, 2020

Choose a reason for hiding this comment

mitchelsellers Jul 6, 2020

Choose a reason for hiding this comment

mitchelsellers Jul 6, 2020

Choose a reason for hiding this comment

X3Technology commented Jul 6, 2020

X3Technology commented Jul 6, 2020

X3Technology commented Jul 7, 2020

valadas commented Jul 7, 2020

X3Technology commented Jul 7, 2020

valadas commented Jul 7, 2020

bdukes commented Jul 10, 2020

bdukes commented Jul 10, 2020

bdukes commented Jul 10, 2020

valadas commented Jul 11, 2020

bdukes commented Jul 13, 2020

valadas commented Jul 13, 2020

bdukes commented Jul 20, 2020

mitchelsellers commented Jul 20, 2020

bdukes commented Jul 20, 2020

valadas commented Jul 23, 2020

mitchelsellers left a comment •

edited

Loading