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.

Sign up for GitHub

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

Jump to bottom

Various Mermaid improvements #18776

Merged
merged 14 commits into from
Feb 16, 2022
Merged

Various Mermaid improvements #18776

merged 14 commits into from
Feb 16, 2022

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Feb 15, 2022 •
edited
Loading

  • Render into iframe for improved security
  • Use built-in dark theme instead of color inversion
  • Remove flexbox attributes, resulting in more consistent size rendering
  • Update API usage and update to latest version
  • Tested in Firefox, Chrome, Safari

Before:
Screen Shot 2022-02-15 at 19 40 05

After:
Screen Shot 2022-02-15 at 20 27 13
Screen Shot 2022-02-15 at 20 27 31

@silverwind
Various Mermaid improvments
f501325 
- Render into iframe for improved security
- Use built-in dark theme instead of color inversion
- Remove flexbox attributes, resulting in more consistent size rendering
- Update API usage and update to latest version
@silverwind silverwind added backport/v1.16 type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI labels Feb 15, 2022
@silverwind silverwind modified the milestone: 1.16.2 Feb 15, 2022
@silverwind
Copy link
Member Author

Labeling for backport because of the security aspect and it's a nice improvement in the rendering as well.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 15, 2022
@silverwind silverwind changed the title Various Mermaid improvments Various Mermaid improvements Feb 15, 2022
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 15, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 15, 2022
@silverwind
Copy link
Member Author

silverwind commented Feb 15, 2022 •
edited
Loading

There is one more improvement to do: mermaid.init renders into the DOM until the JS picks it out and puts it into the <iframe>. Need to find a solution that does not go via DOM like that. Putting to draft until then.

@silverwind silverwind marked this pull request as draft February 15, 2022 22:05
@Gusted
Copy link
Contributor

Gusted commented Feb 15, 2022

There is one more improvement to do: mermaid.init renders into the DOM until the JS picks it out and puts it into the <iframe>. Need to find a solution that does not go via DOM like that. Putting to draft until then.

Would it be possible to create the <iframe> beforehand and create the necessary SVG already and then pass that as "node" into the init function, the only thing you would need to do afterwards is to set the correct height style, but that should avoid to use the parent document instead of the iframe document.

@silverwind silverwind marked this pull request as ready for review February 16, 2022 00:17
@silverwind
Copy link
Member Author

silverwind commented Feb 16, 2022 •
edited
Loading

Would it be possible to create the <iframe> beforehand and create the necessary SVG already and then pass that as "node" into the init function, the only thing you would need to do afterwards is to set the correct height style, but that should avoid to use the parent document instead of the iframe document.

Exactly my thoughts, fixed it. With the new method, I needed to parse the height from the SVG string, which is a bit clumsy, but should be faster than parsing the whole thing again using a DOMParser or similar.

Of note there is one tidbit to mermaidAPI.render, one is instructed to bind JS events via second argument bindFunctions provided in the callback, but we can't do that without loading the whole library into each iframe. I don't think we'd actually need this feature thought as it is disabled in strict security mode anyways, and there is no way for the user to input JS code anways.

@silverwind silverwind added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Feb 16, 2022
@lunny
Copy link
Member

lunny commented Feb 16, 2022

make L-G-T-M work.

@lunny lunny merged commit 616146f into go-gitea:main Feb 16, 2022
@lunny
Copy link
Member

lunny commented Feb 16, 2022

Please send backport

silverwind added a commit to silverwind/gitea that referenced this pull request Feb 16, 2022
@silverwind @lunny
Various Mermaid improvements (go-gitea#18776)
f02e245 
* Various Mermaid improvments

- Render into iframe for improved security
- Use built-in dark theme instead of color inversion
- Remove flexbox attributes, resulting in more consistent size rendering
- Update API usage and update to latest version

* restart ci

* misc tweaks

* remove unneccesary declaration

* make it work without allow-same-origin, add loading=lazy

* remove loading attribute, does not seem to work

* rename variable

* skip roundtrip to DOM for rendering

* don't guess chart height

* update comment to make it clear it's intentional

* tweak

* replace deprecated 'scrolling' property

* remove unused css file

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@silverwind silverwind mentioned this pull request Feb 16, 2022
Merged
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 16, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
cce9d48 
* giteaofficial/main:
  Various Mermaid improvements (go-gitea#18776)
  [skip ci] Updated translations via Crowdin
  Fix display time of milestones (go-gitea#18753)
lunny added a commit that referenced this pull request Feb 16, 2022
@silverwind @lunny
Various Mermaid improvements (#18776) (#18780)
1f7802d 
* Various Mermaid improvments

- Render into iframe for improved security
- Use built-in dark theme instead of color inversion
- Remove flexbox attributes, resulting in more consistent size rendering
- Update API usage and update to latest version

* restart ci

* misc tweaks

* remove unneccesary declaration

* make it work without allow-same-origin, add loading=lazy

* remove loading attribute, does not seem to work

* rename variable

* skip roundtrip to DOM for rendering

* don't guess chart height

* update comment to make it clear it's intentional

* tweak

* replace deprecated 'scrolling' property

* remove unused css file

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@silverwind silverwind deleted the mermaid-improve branch February 16, 2022 09:52
Caellion added a commit to Caellion/gitea that referenced this pull request Feb 16, 2022
@Caellion
Merge branch 'main' of https://github.com/go-gitea/gitea
8adf230 
* 'main' of https://github.com/go-gitea/gitea: (87 commits)
  Fix template bug of LFS lock (go-gitea#18784)
  Various Mermaid improvements (go-gitea#18776)
  [skip ci] Updated translations via Crowdin
  Fix display time of milestones (go-gitea#18753)
  [skip ci] Updated translations via Crowdin
  Prevent dangling GetAttribute calls (go-gitea#18754)
  Add example to render html files (go-gitea#18736)
  Fix a broken link in `commits_list_small.tmpl` (go-gitea#18763)
  Fix broken cancel button link on patch page (go-gitea#18718)
  Ignore the migrate if u2f_registration is not exist (go-gitea#18760)
  [skip ci] Updated translations via Crowdin
  Increase the size of the webauthn_credential credential_id field 
(go-gitea#18739)
  Fix isempty detection of git repository (go-gitea#18746)
  [skip ci] Updated translations via Crowdin
  Send mail to issue/pr assignee/reviewer also when OnMention is set 
(go-gitea#18707)
  Reduce CI go module downloads, add make targets (go-gitea#18708)
  Add number in queue status to monitor page (go-gitea#18712)
  Fix source code line highlighting (go-gitea#18729)
  Fix forked repositories missed tags (go-gitea#18719)
  [skip ci] Updated translations via Crowdin
  ...
@zeripath zeripath added the backport/done All backports for this PR have been created label Mar 9, 2022
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
@silverwind @lunny
Various Mermaid improvements (go-gitea#18776)
6fe4d06 
* Various Mermaid improvments

- Render into iframe for improved security
- Use built-in dark theme instead of color inversion
- Remove flexbox attributes, resulting in more consistent size rendering
- Update API usage and update to latest version

* restart ci

* misc tweaks

* remove unneccesary declaration

* make it work without allow-same-origin, add loading=lazy

* remove loading attribute, does not seem to work

* rename variable

* skip roundtrip to DOM for rendering

* don't guess chart height

* update comment to make it clear it's intentional

* tweak

* replace deprecated 'scrolling' property

* remove unused css file

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@wxiaoguang wxiaoguang added this to the 1.17.0 milestone Mar 29, 2022
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zeripath zeripath zeripath approved these changes

@Gusted Gusted Awaiting requested review from Gusted

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.
6 participants
@silverwind @Gusted @lunny @zeripath @wxiaoguang @GiteaBot