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

<<Video ..., auto=true>> to auto-play inline video (GIF style) #442

Merged
merged 3 commits into from
Mar 21, 2023

Conversation

aljungberg
Copy link
Contributor

…le).

- Using the `="true"` work-around as in gollum#430.
- The change in `sanitization.rb` further addresses gollum/gollum#1587 by allowing more of the standard video properties.
- Also featured: less escaping, capitalisation fix.
@dometto dometto closed this Mar 12, 2023
@dometto dometto reopened this Mar 12, 2023
@dometto
Copy link
Member

dometto commented Mar 12, 2023

(Closed and reopened to trigger test run.)

Thanks @aljungberg and sorry for the very slow reply. These additions and changes make sense to me. Could you just update the test expectations (see CI test runs) and add an expectation for the new autoplay property?

It would also be greatly appreciated if you could update the wiki to include the new feature!

@aljungberg
Copy link
Contributor Author

Thanks for reviewing! I've made the changes and included the feature in the wiki.

@dometto
Copy link
Member

dometto commented Mar 16, 2023

Thanks again! Sadly it looks like the JRuby tests fail because the markdown parser on JRuby generates <p> tags around the video tag. I think this is nothing to worry about, you could just make the expectation regex more lenient.

@aljungberg
Copy link
Contributor Author

Hmm I’m not sure, isn’t the regex a find rather than a match? It feels like the first test should have failed the same way if extra <p>s were the problem, but it seems only the second test is failing. Also looking for that space to %20 escape and I’m not seeing that in the actual output, instead the space seems to have been silently swallowed. Something different about how JRuby parses the macro tag such that spaces in filenames get eaten?

</<video (.*)src="\/Uploads\/foo%20.mp4"(.*)>(.*)<\/video>/> was expected to be =~
[17](https://github.com/gollum/gollum-lib/actions/runs/4436685631/jobs/7788167501?pr=442#step:6:18)
  <"<p><video autoplay=\"true\" height=\"100%\" loop=\"true\" muted=\"true\" playsinline=\"true\" src=\"/Uploads/foo .mp4\" width=\"100%\">HTML5 video is not supported on this browser.</video></p>\n">.

@dometto
Copy link
Member

dometto commented Mar 20, 2023

Ah, you're right about the location of the problem! It seems that CGI.escapeHTML does not convert spaces to %20 on either JRuby or MRI. So the %20 on MRI must be getting produced somewhere else in gollum, probably due to a difference in the rendering gems between MRI./JRuby. Maybe we can just change the example filename to test escaping, e.g. to something like /Uploads/file<script>.mp4?

jruby-9.4.0.0 :004 > CGI.escapeHTML("/Uploads/file<script>.mp4")
 => "/Uploads/file&lt;script&gt;.mp4" 

@aljungberg
Copy link
Contributor Author

Yep works for me! Pushed.

@dometto dometto merged commit 1f47c03 into gollum:master Mar 21, 2023
@dometto
Copy link
Member

dometto commented Mar 21, 2023

Thanks for your effort!

@aljungberg aljungberg deleted the autovideo branch March 21, 2023 21:59
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.

2 participants