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

JavaScript library does not natively support markdown, and third-party libs are too permissive #910

Closed
riarenas opened this issue Nov 8, 2017 · 14 comments
Assignees
Labels
Area-Inconsistency Bugs around renderer inconsistencies across different platforms Area-Markdown Area-Renderers Bug Platform-JavaScript Bugs or features related to the JavaScript renderer

Comments

@riarenas
Copy link
Member

riarenas commented Nov 8, 2017

This sample has a lot of markdown syntax that shouldn't be supported according to the docs.

{
    "$schema": "http://adaptivecards.io/schemas/adaptive-card.json",
    "type": "AdaptiveCard",
    "version": "1.0",
    "body": [
        {
            "type": "TextBlock",
            "text": "## Markdown Headers should not be supported"
        },
        {
            "type": "TextBlock",
            "text": "___"
        },
        {
            "type": "TextBlock",
            "text": "Neither should horizontal rules"
        },
        {
            "type": "TextBlock",
            "text": "Some of my spaces are being eaten up by the         markdown parser"
        },
        {
            "type": "TextBlock",
            "text": "~~Strikethrough should not be supported.~~"
        },
        {
            "type": "TextBlock",
            "text": ">Quote blocks should be ignored"
        },
        {
            "type": "TextBlock",
            "text": "Markdown | Tables | Work\n:--- | :---: | ---:\n*And* | change | **horizontal alignment**\n1 | 2 | 3"
        },
        {
            "type": "TextBlock",
            "text": "    This one I don't really understand"
        },
        {
            "type": "TextBlock",
            "text": "````\nBut Seems to be a shortcut for code blocks, which should be ignored."
        }
    ]
}

Renders as:

image

@riarenas riarenas added Area-Renderers Bug Platform-JavaScript Bugs or features related to the JavaScript renderer labels Nov 8, 2017
@riarenas
Copy link
Member Author

riarenas commented Nov 8, 2017

Inline images also work via markdown:

{
    "$schema": "http://adaptivecards.io/schemas/adaptive-card.json",
    "type": "AdaptiveCard",
    "version": "1.0",
    "body": [
        {
            "type": "TextBlock",
            "text": "![alt text](https://github.com/adam-p/markdown-here/raw/master/src/common/images/icon48.png \"Logo Title Text 1\")"
        }
    ]
}

renders as

image

@dclaux
Copy link
Member

dclaux commented Nov 10, 2017

This one is interesting:

  • Best way to do this would be to tell the Markdown processing library to not handle some aspects of Markdown. I am not sure such a library exists
  • Even if it does, we have been talking about removing the Markdown-it (or any other markdown library)dependency from the TypeScript renderer and instead leave it to the host to process markdown. But then it becomes the responsibility of every host to only support the subset of markdown Adaptive is supposed to allow

So, how can we address this?

@matthidinger
Copy link
Member

For .NET we forked a library and customized it. For the iOS/Android/UWP we are working through options, including writing a shared parser/implementation in our C++ layer. There are plenty of references out there, it seems like rolling our own given the very limited set of markdown options might be the best way to go?

@synk
Copy link

synk commented Nov 24, 2017

we've found implementing markdown subset is not easy work 😫
even if the spec becomes clear, it is difficult to use existing library as is.

I think adaptivecards is expressive enough even without markdown support
e.g.

  • selectAction instead of markdown hyperlink
  • facts and columns instead of markdown list

on the other hand, i hope auto linebreak and inline text emphasis feature since these are most wanted features by our developers.
but other markdown features can be replaced by adaptivecards itself.

@shalinijoshi19
Copy link
Member

@andrewleader / @matthidinger / @paulcam206/ @dclaux is this addressed with 1.2 changes all up across all platforms? What should be the expected behavior wrt markdown in textblocks and richtextblocks resp with 1.2?

@andrewleader
Copy link
Contributor

No, this isn't addressed, but it's something we need to do. This inconsistent behavior leads to confusion. Users expect that what you see in the designer is what you get everywhere else.

@dclaux
Copy link
Member

dclaux commented Apr 15, 2019

Problem is we may not be able to control it unless we provide a built-in Markdown processor. We don't load markdown-it, and I'm not sure we can reconfigure it after it's been loaded by the host application. Plus, using Markdown-it isn't a requirement.

@shalinijoshi19
Copy link
Member

I'll table this in that case for 1.3 addressing. @andrewleader what are teh next steps here for this to result in a Markdown-related requierments spec for 1.3 ? THanks!

@shalinijoshi19
Copy link
Member

Working with @andrewleader to create a "parent" "Markdown support in TextBox" task/issue for 1.3

@shalinijoshi19
Copy link
Member

@dclaux FYI likely moving this to 1.3 (adding a checkbox that lets textbox element have the option to "opt-out" of markdown

@matthidinger
Copy link
Member

FYI the ReactNative team did in fact implement their own markdown processor. @Vasanth-S can provide details. Ideally we could use the same or similar code in the plain JS renderer to bundle official AC markdown support in the renderer

@Surezt22
Copy link

Surezt22 commented Apr 17, 2019

@Vasanth-S / @matthidinger This is the markdown parser we are using for our react-native implementation. Currently, it supports the below 5 types:

  • Bold
  • Italic
  • Ordered List
  • Unordered List
  • Link

You can refer the same to support web-based markdown processor. In case, if you need any help, we are happy to work on that.

@matthidinger matthidinger changed the title Typescript markdown syntax is too permissive. JavaScript library does not natively support markdown, and third-party libs are too permissive May 14, 2019
@andrewleader andrewleader added the Area-Inconsistency Bugs around renderer inconsistencies across different platforms label Jun 12, 2019
@shalinijoshi19 shalinijoshi19 added this to the Backlog milestone Jun 19, 2019
@shalinijoshi19
Copy link
Member

No immediate requirement to do this at this point. Chatted with @dclaux ; moving this out to our backlog to revisit.

@jonmill
Copy link
Contributor

jonmill commented Nov 4, 2021

Tracked by #1984

@jonmill jonmill closed this as completed Nov 4, 2021
@ghost ghost removed the Triage-Approved for Fix label Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Inconsistency Bugs around renderer inconsistencies across different platforms Area-Markdown Area-Renderers Bug Platform-JavaScript Bugs or features related to the JavaScript renderer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants