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

Support Markdown in Add-on Listing Fields #22956

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

chrstinalin
Copy link
Contributor

@chrstinalin chrstinalin commented Dec 20, 2024

Fixes: mozilla/addons#15145

Description

Replaces HTML syntax support with Markdown support in the description, developer_comments, eula, and privacy_policy fields of Addon. Existing HTML in those fields will remain functional until the field is edited. No new HTML will be functional.

Only the markdown-equivalent of currently allowed HTML attributes are allowed. abbr uses the PHP Markdown Extra syntax.

DevHub:

image
image
image

Frontend:

image
image
image

Testing

New Add-ons

  1. Create a listed add-on.
  2. Under 'submit/details', description should have the tooltip and support markdown.

Existing Add-ons

  1. Navigate to an add-on that already has HTML in its description. The HTML should still be functional. The HTML is still rendered on the frontend listing.
  2. Under the 'edit' page, the description and developer comments should support markdown syntax.*
  3. Under the 'Manage Authors & License' page, the EULA and Privacy Policy fields should support markdown syntax.*

* Any user-inputted HTML saved at this point is rendered as plaintext. Any allowed markdown in the field is rendered as expected in the frontend. Disallowed markdown is stripped from the result and not visible at all (this is to avoid showing the user the markdown formatted into HTML).

Checklist

  • Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
  • Successfully verified the change locally.
  • The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.
  • Add before and after screenshots (Only for changes that impact the UI).
  • Add or update relevant docs reflecting the changes made.

@chrstinalin chrstinalin marked this pull request as draft December 20, 2024 21:05
@chrstinalin chrstinalin marked this pull request as ready for review December 24, 2024 17:33
@chrstinalin chrstinalin requested review from a team and KevinMind and removed request for a team December 24, 2024 17:41
@chrstinalin
Copy link
Contributor Author

There should probably be a more informative way to explain what syntax or Markdown is supported, possibly similar to how GitHub explains it via the 'Markdown is Supported' button by navigating to a dedicated page.

Copy link
Contributor

@KevinMind KevinMind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks promising. A few comments before testing.

src/olympia/translations/models.py Show resolved Hide resolved
src/olympia/devhub/templates/devhub/includes/macros.html Outdated Show resolved Hide resolved
src/olympia/devhub/templates/devhub/includes/macros.html Outdated Show resolved Hide resolved
src/olympia/translations/models.py Show resolved Hide resolved
@KevinMind
Copy link
Contributor

Please add testing steps and I can verify the PR.

@chrstinalin chrstinalin requested a review from KevinMind January 8, 2025 14:39
@KevinMind
Copy link
Contributor

@chrstinalin what should I expect to happen if I enter invalid or disallowed markdown syntax? Can you add that to the test scenario. Additionally can you clarify what "functional" means? I think you mean the syntax should be "rendered" on the page right?

@chrstinalin chrstinalin requested a review from KevinMind January 9, 2025 16:08
@KevinMind
Copy link
Contributor

After modifying existing html I can confirm:

  • before modification the html is rendered
  • after the modification it is stringified
image

Should we expect the "stringified" href attribute to still be rendered as a link though? it is.. I'm not sure if that is expected or not.

@KevinMind
Copy link
Contributor

I'm seeing a template tag instead of actually allowed markdown tags

image

@KevinMind
Copy link
Contributor

Do I still expect to see html supported for version notes?

image

@KevinMind
Copy link
Contributor

@chrstinalin I cannot find the developer comments section.. can you help navigating there?

@KevinMind
Copy link
Contributor

There is no way to preview content in the authors page. I can add markdown but after saving I still see the textarea so I have no idea what it would actually render as

image

@KevinMind
Copy link
Contributor

@chrstinalin something that would be very helpful when verifying this is if you could share a sample of both html and markdown that include a sample of all the supported tags.

I even wonder if having this available to developers on a page could be useful. There are many flavors of markdown and looking at the tag, it's hard for me (with context) to know what and how I should input the syntax to get the expected result.

Imagine how difficult it would be for a developer.

Copy link
Contributor

@KevinMind KevinMind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still needs some work. A few bugs discovered and some edge cases needing addressing.

Also found a UX hangup that might be worth addressing in terms of usability of this feature.

@chrstinalin
Copy link
Contributor Author

chrstinalin commented Jan 20, 2025

Should we expect the "stringified" href attribute to still be rendered as a link though? it is.. I'm not sure if that is expected or not.

Currently, if you were to put just a link in, it'd render similarly. If we're essentially treating user HTML as plaintext from now on, I can't see why this wouldn't be expected?

Do I still expect to see html supported for version notes?

I had omitted it originally since it wasn't part of the original issue. Now that I think about it, though, there's probably no reason to do that. @abyrne-moz should add-on version notes also make the switch from HTML to Markdown? Got the OK through Slack

I cannot find the developer comments section.. can you help navigating there?

At the very bottom of 'Edit Product Page', under 'Technical Details'.

There is no way to preview content in the authors page. I can add markdown but after saving I still see the textarea so I have no idea what it would actually render as

As it did with HTML before, too. It did bother me, though. It shouldn't hurt to just add the rendering (though it might warrant a separate PR -- especially since this behaviour spans several pages; like 'manage version', too). Made an issue here mozilla/addons#15293

something that would be very helpful when verifying this is if you could share a sample of both html and markdown that include a sample of all the supported tags.

Some of these words *are emphasized*.
Some of these words _are emphasized also_.

Use two asterisks for **strong emphasis**.
Or, if you prefer, __use two underscores instead__.

Here's a link: [Google](https://www.google.com "Click me!").

**abbr, acronym**

The HTML specification is maintained by the W3C.
*[HTML]: Hyper Text Markup Language
*[W3C]:  World Wide Web Consortium

**codeblocks, blockquotes**

Here is code:

    tell application "Foo"
        beep
    end tell

> This is the first level of quoting.
> > This is nested blockquote.

**Lists**

*Ordered List:*

1. First item
2. Second item
3. Third item

*Unordered List:*

- Item 1
- Item 2
- Item 3

This is an [example link](http://example.com/)

<a href="www.google.com">This shouldnt work!</a>

<ol>
<li>Neither</li>
<li>Should</li>
<li>This</li>
</ol>

I even wonder if having this available to developers on a page could be useful. There are many flavors of markdown and looking at the tag, it's hard for me (with context) to know what and how I should input the syntax to get the expected result.

Yeah, which is why I mentioned it before. Though I imagined something like:

  1. Merge this PR
  2. Merge a PR to extension-workshop with syntax information (Include Markdown Syntax Information extension-workshop#1994)
  3. Create a PR to link to said information.

Being a smoother way about it?

@KevinMind
Copy link
Contributor

Thanks for the updates. Everything checks out except this part:

Currently, if you were to put just a link in, it'd render similarly. If we're essentially treating user HTML as plaintext from now on, I can't see why this wouldn't be expected?

If I render this on a browser:

<html>
<body>
<a href="http://github.com/mozilla/addons-server">
Github repo
</a>
</body>
</html>

I do not expect a clickable link to appear on the screen. I expect (just like github is doing, to just render out all the markup as plain text.

I kind of have the same expectations for our "plain text" html where we render links. As you can see the html syntax is rendered as plain text, but somehow, the link is a clickable link.. that seems odd.

Is that actually expected behaviour? why plaint text the links then?

@chrstinalin
Copy link
Contributor Author

chrstinalin commented Jan 20, 2025

@KevinMind I see what you mean now -- I do agree its a bit weird. But I'd still say its expected here just as a result of the linkify bleach converting any string that looks like a URL, and the additional behaviour it provides still being necessary.

I'm also a bit hesitant to the idea of doing a hack around this, since bleach is deprecated and there's a separate issue to replace it completely... I don't have any strong opinions on that, though, and I can still see arguments against that.

I also asked Andrew about it, who agrees that it's not a problem (plaintext URLs should be linkified, and its hard to differentiate those from HTML ones. If anything, to make fixing it a follow-up).

@KevinMind
Copy link
Contributor

KevinMind commented Jan 22, 2025

Do you mean the "whiteboard" if that also supports markdown I would expect the same tooltip we have in the other inputs.

FIXED: I see the issue, I'm using a non-listed version that does not display this field.

image

Copy link
Contributor

@KevinMind KevinMind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the specified input sources as correctly formatting markdown and plaint text ifying html

image

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

Successfully merging this pull request may close these issues.

[Task]: Support markdown in add-on listing fields
2 participants