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

Port of MarkdownTag to a WebComponent #7

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

DarkenLM
Copy link
Collaborator

Ported MarkdownTag to a WebComponent. Allows for a single script to use all three parsers.
Added documentation for MarkdownTag.
Fixed XSS Vulnerability, as far as the DOM Structure allows to.
Added XSS Vulnerability Tester.

Fixes #5

DarkenLM added 7 commits May 23, 2022 23:56
Added Documentation
Added Local Script Copy Fallbacks
- Modified WebComponent - Added usage of nested textarea element for
stopping XSS (Likely to be reverted, easily bypassed)
- Added MdTagDebugger - built-in debugger for easily debugging
WebComponent at runtime.
Modified and updated Documentation.
Added XSS Vulnerability Tester.
Updated README
Updated to-do
@MarketingPip
Copy link
Member

MarketingPip commented Jun 2, 2022

@DarkenLM - would you be willing to take on this feature request / to-do list job? Support code syntax highlighting for GFM

I created this demo repo https://marketingpip.github.io/Syntax_Demo/ to go off creating a JS function for - when done looking at the demo please let me know so I can delete the repo.

For all content inside <md hightlight="github-syntax" type="js">alert("hello")</md>
will need converted using https://github.com/primer/github-syntax-theme-generator/
and results placed back into self like so <md> <syntax-hightlight> WHATEVER INPUT / VALUE USER HAD IN TAG </syntax-hightlight> </md>

If willing to do this let me know - tho, you've already more than out done yourself on these commits. Excited to push them ASAP once we make some final decision's for next version of the project & continue make this project even better for the community.

@MarketingPip
Copy link
Member

PS - if you're willing to do this - I'll try to organize the rest of the GitHub Markdown Flavour CSS in the mean time to provide you with!

@MarketingPip
Copy link
Member

MarketingPip commented Jun 4, 2022

Notice for this PR - we need to test script loading / payload times via SEO audits etc. Currently times pass acceptable times for SEO score. Plus to improve things - styles should not be applied in-line but to body head to improve SEO on page. As of right now they are NOT in current version of Markdown Tag and is currently slightly affecting page scores.

@DarkenLM
Copy link
Collaborator Author

DarkenLM commented Jun 5, 2022

The reason stylesheets are loaded into the shadow DOM is to provide encapsulation, allowing for each MarkdownTag instance to be affected exclusively by it's own properties, however, I think I can add the stylesheets to the body head, as due to the stylesheet only applying styles to <github-md> tags, the script wraps the github markdown in a <github-md> tag, which would not sacrifice encapsulation.

@MarketingPip
Copy link
Member

The reason stylesheets are loaded into the shadow DOM is to provide encapsulation, allowing for each MarkdownTag instance to be affected exclusively by it's own properties, however, I think I can add the stylesheets to the body head, as due to the stylesheet only applying styles to <github-md> tags, the script wraps the github markdown in a <github-md> tag, which would not sacrifice encapsulation.

Don't know how I missed this! But yes - adding to the head is a better solution!

Was playing with some things for syntax highlighting. Don't know why I suggest that poor option for highlighting before - we should be using the same style for GitHub uses so content can be highlighted and non highlighted like so

GitHub Hightlight Example

````ruby
   require 'redcarpet'
   markdown = Redcarpet.new("Hello World!")
   puts markdown.to_html
````

And a better resource then what I posted before - my apologizes in advanced!

GitHub Syntax Highlighting

@MarketingPip
Copy link
Member

@DarkenLM @shaunbharat - again my apologizes. Marked already has highlighter options & Showdown has a plugin that we can add in. So no need to build these options from the ground up.

Using Advanced - Marked Documentation

Showdown Highlight: A Showdown extension for highlighting code blocks.

@MarketingPip
Copy link
Member

MarketingPip commented Jun 9, 2022

@DarkenLM @shaunbharat - playing around again & https://github.com/PrismJS/prism does auto-detection on page already in same format as GitHub. This will be the option we will add in. Literally can just be added into the script.

@AR-Student824
Copy link

@MarketingPip when will the syntax highlighting feature be implemented?

@MarketingPip
Copy link
Member

@AR-Student824 - hoping to go over some options before implementing this PR request to see if possible to keep the <md> tag vs having to use <mark-down> - by then Syntax Highlighting should be implemented.

For now you can currently add https://github.com/PrismJS/prism to your web page tho & it will work!

@MarketingPip
Copy link
Member

@DarkenLM - can I ask you to change the file structure of this branch to match the following?

Example:

version/1.0.0/dist/markdown-tag.min.js
version/1.0.0/src/markdown-tag.js
version/1.0.0/docs/

As well possibility the "tests" folder moved into

.github

As well @shaunbharat - is there any changes to grammar, README etc that you might want to commit if you have not already?

@DarkenLM
Copy link
Collaborator Author

DarkenLM commented Jul 6, 2022

Will do as soon as I can.
However, it might take a while, as I found some cases where the webcomponent is broken and I'm currently trying to fix it.

Also, was the Github CSS file modified lately? It seems to be broken suddenly.

Sorry for the edits. Found the bug.
Changing

github-md h1, h2, h3

to

github-md h1, github-md h2, github-md h3

solves the problem.

Updated WebComponent
Updated File Strucutre
@DarkenLM
Copy link
Collaborator Author

DarkenLM commented Jul 6, 2022

@MarketingPip Done. Added a build script for convenience using UglifyJS.

@shaunbharat
Copy link
Collaborator

@MarketingPip I can open a PR for the changes I have at the moment, but I haven't made any new major changes in a while

Currently, I just have minor improvements to the readme - @DarkenLM can I just open a PR for your fork?

@MarketingPip
Copy link
Member

Will do as soon as I can. However, it might take a while, as I found some cases where the webcomponent is broken and I'm currently trying to fix it.

Also, was the Github CSS file modified lately? It seems to be broken suddenly.

Sorry for the edits. Found the bug. Changing

github-md h1, h2, h3

to

github-md h1, github-md h2, github-md h3

solves the problem.

@DarkenLM - not that I know of? It appears CSS is working fine on my end for GitHub style sheet without those changes applied.

Two things tho - assuming you have tested your HTML Sanitizer functions with more than one test? As some sanitizers will sanitize TOO much content & mess things up. Tho ShowDown does have a sanitizer built in (correct me if wrong) - just doesn't work 100% correctly.

As well assuming you have looked into using Prism JS in your fork as it provides support for syntax highlighting? Tho keep in mind we do need to find a good CSS theme for Prism JS that is as close to the GitHub syntax highlighting color styles. Maybe this is something you can overlook for options @shaunbharat?

When you think thing's are finally ready @DarkenLM - please provide some test suites so we can keep this project more professional going fourth. If needed - I can write some basic one's up when you have thing's ready.

@shaunbharat - I will ask you to review the options avaliable for CSS themes, as well over look any grammar / spelling / improvements that can be made in DarkenLM's fork.

And if willing / able - review the code pushed, see if anything sticks out to you that might need changed. As well do some live tests to make sure thing's are working smoothly (I will do the same tasks as well) & once we have confirmed thing's are ready. I will accept the pull request with the updated changes! 🎉

Let's get this done sooner than later! 👍

ps; Thank you both for your commitment to this project! And hope you are both doing more than well!

@MarketingPip
Copy link
Member

MarketingPip commented Jul 7, 2022

@DarkenLM - have not personally done a usage test of your fork but your code is more than GOLD 🥇

Was over-looking your XSS test suite as well - and am more than likely assuming I should be leaving the test suites to you at this time. 😆 Tho I can provide some basic one's.

Few thing's I wanted to address one being a small one - assuming this was typo or short form for "Assert". But thinking we should change the following debug line's here lol!

       [APPLY] [aSS] 

to

      [APPLY] [CSS]

Second thing regarding file paths in repo we will need to move the stylesheets folder to version/1.0.0/src/ as well make another stylesheets folder in version/1.0.0/dist/ with the minified CSS. And adjust the file path's in the code to match the updated folder paths.

tests can become test_suites & test type(s) can be organized by folder like the following example test_suites/XSS_Testing

Third thing - assuming this was a typo error etc. But in the README you do not have HTML decoding checked off...? Tho I will want to be going through the README & provide some changes based on github.com/MarketingPipeline repo style's & make some improvements to it as well / remove some un-necessary bits. And then I will have @shaunbharat review it for grammar + suggest any changes & you can follow up from there.

Plus one extra thing - we can remove the wc from the .js file name.

I do believe @shaunbharat has already went through the docs and made improvements tho as well - but maybe @shaunbharat wants to add something after personal use-age of the updated version etc that user's might find useful etc.

We're so close to releasing this update 🥳

@MarketingPip
Copy link
Member

MarketingPip commented Jul 7, 2022

@DarkenLM as well for indentation issue / improvement.

Do not know if this would be a capable fix but as you said - maybe we could get first character in line and move all content under first character to the left side of HTML document automatically.

Example :

      <md-tag>
     # Content 

                 Code Block Content
     </md-tag>

would automatically become

      <md-tag>
# Content 

      Code Block Content
     </md-tag>

Moving all content to left like so. I don't know if counting characters to remove based on the white space of first character is best idea to go about doing this or if there is some kind of align like library / feature we can use that exists already for this (that will not mess with user content).

@DarkenLM
Copy link
Collaborator Author

DarkenLM commented Jul 7, 2022

Few thing's I wanted to address one being a small one - assuming this was typo or short form for "Assert". But thinking we should change the following debug line's here lol!

That aSS is the initials of assertStyleSheet. I didn't even realized what it said when I wrote it and I'm currently dying laughing.

Second thing regarding file paths in repo we will need to move the stylesheets folder to version/1.0.0/src/ as well make another stylesheets folder in version/1.0.0/dist/ with the minified CSS. And adjust the file path's in the code to match the updated folder paths.

The build script already copies any non-source file to it's respective place on the dist directory ;)

tests can become test_suites & test type(s) can be organized by folder like the following example test_suites/XSS_Testing

Will do

Third thing - assuming this was a typo error etc. But in the README you do not have HTML decoding checked off...? Tho I will want to be going through the README & provide some changes based on github.com/MarketingPipeline repo style's & make some improvements to it as well / remove some un-necessary bits. And then I will have @shaunbharat review it for grammar + suggest any changes & you can follow up from there.

I didn't touch the README too much, just enough to explain my changes and provide an easy way to understand them.

Plus one extra thing - we can remove the wc from the .js file name.

Will do.

Do not know if this would be a capable fix but as you said - maybe we could get first character in line and move all content under first character to the left side of HTML document automatically.

You are a very lucky man. I made a function to dedent strings some hours ago, but completely forgot MarkdownTag required it.

@MarketingPip
Copy link
Member

MarketingPip commented Jul 7, 2022

@DarkenLM figured you'd get a laugh too!

Doing some tests on my end here - on iPhone 5S (don't laugh bout that either lol)

The documentation example does not load. Have not tested if only the documentation or if this web component does not load properly at all on Mobile Devices.

image

Image attached for reference.


As for the file path builds - you mentioned the files are moved upon build tho I would like to move them just to keep the repo structure as clean as possible.

Regarding this as well - maybe you can give me a refresher but is there not a way to link different repos / branches in the same repo via a folder? (On click of folder on GitHub.com it redirects to another branch)

I thought GitHub had this feature but I can't recall how to do it, just thinking as much as I would like to keep all the versions together in one main branch. If this project ever does have future versions, for someone to download the source code they would literally be downloaded every version produced (which is NOT ideal obviously)


And haha that's just our intuition speaking 😄

But correct - very lucky that you had that already done, as well more than lucky to have you + @shaunbharat collaborating on the project in general ☺️

Excited to see where this could become in the future!


ps; @DarkenLM - don't know if you read the ShowDown docs / README page lately. But they did add some more flavour values (that I did not see previously before). You might want to make a call to remove a parser and replace it with showdown to make the script smaller - tho I will leave that judgement call up to you & you solely.

@MarketingPip
Copy link
Member

@DarkenLM - confirming issue on iOS 13 with this PR.

Script does NOT load properly (script id's for parser are not placed in document) on iOS 13 / Apple iPhone. Can not confirm for other devices.

@MarketingPip MarketingPip added the bug Something isn't working label Jul 8, 2022
@DarkenLM
Copy link
Collaborator Author

DarkenLM commented Jul 8, 2022

Regarding this as well - maybe you can give me a refresher but is there not a way to link different repos / branches in the same repo via a folder? (On click of folder on GitHub.com it redirects to another branch)

The only thing I can remember are git submodules, but I am not that familiar with them either.

You might want to make a call to remove a parser and replace it with showdown to make the script smaller

The script downloads the files as they are needed, so it's not that big of a deal, fortunately.

Script does NOT load properly (script id's for parser are not placed in document) on iOS 13 / Apple iPhone

Well that is unfortunate, as I have no idea what is failing. I know Apple does not like to follow standards, but for what I checked, all API calls used are supported by Safari.

@MarketingPip
Copy link
Member

@DarkenLM - I would assume this has something to do with the web component. As the tag version (current version) works with mobile rendering. I will have to do some test to confirm later on this tho some input would be more than appreciated.

@MarketingPip
Copy link
Member

Leaving this here as well.

CSS stylesheet needed for this project:

starry-night: Syntax highlighting, like GitHub

@MarketingPip
Copy link
Member

MarketingPip commented Jul 31, 2022

@DarkenLM - confirming on iOS 13 there is no support for basic web components. We will need to add a fix for this and then should be able to merge this PR. I will be working on new documentation ASAP that falls under open source license so no conflicting license's with this repo.

Tho you are going to hate me - as I found a similar project to this one & they are already using method that requires a fix for FOUC before rendering. Which again - wondering if we could possibly use this method & keep the md tag with the suggestion provided earlier. This is just a thought tho!

Edit: Correcting myself, it appears constructed style sheets do not work. Tho the current version of Markdown-Tag does work on iOS 13.

@MarketingPip
Copy link
Member

@DarkenLM - my apologizes for leaving this hanging so long!

I still want to add this to the branch / repo!

I did find the correct polyfills needed for the web component.

Tho I would still like to address some things / then make this happen!

Hope you're still on-board + @shaunbharat is as well to get this merged 👌 sorry for any delays on my part guys!

@shaunbharat
Copy link
Collaborator

I'm still onboard and would like to help wherever I can! I started my school year a while back though, so I haven't even touched a code editor for a while.

@DarkenLM
Copy link
Collaborator Author

I definitely want to keep contributing to this repo. Unfortunately, my university also started, so my free time is greatly reduced, so contributions might be slower.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Tag(s) added after page load do not render
4 participants