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

rst: use CSS for image alignment. #1222

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

Conversation

flying-sheep
Copy link
Contributor

fixes #163

@kayyymarieee
Copy link

content = File.read('test2.txt')
detection = CharlockHolmes::EncodingDetector.detect(content)
utf8_encoded_content = CharlockHolmes::Converter.convert content, detection[:encoding], 'UTF-8'

@kayyymarieee
Copy link

9173626

@terencehonles
Copy link
Contributor

@flying-sheep do you know if any of the other options are not working? You may want to update the branch so the CI can re-run (it now uses GitHub actions).

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Apr 3, 2021

Well, you use the HTML::Pipeline::SanitizationFilter without options:

https://github.com/github/markup/blob/master/test/markup_test.rb#L44-L47

This means that these defaults are used: https://github.com/gjtorikian/html-pipeline/blob/0e3d84eb13845e0d2521ef0dc13c9c51b88e5087/lib/html/pipeline/sanitization_filter.rb#L133

Changing the config in the tests here won’t change what GitHub does.

And the defaults are bad: They allow the obsolete align on all elements, but not style="float: left".

We have to make GitHub parametrize its use of HTML::Pipeline::SanitizationFilter to allow some specific styles. Or update the defaults of that library to allow that: gjtorikian/html-pipeline#302

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Apr 3, 2021

Regarding you question: alt and target work, those do not:

  • width and height are done as CSS and stripped by sanitation (boo! But we could make this library emit width and height attributes instead)
  • scale is ignored by the rst2html command in the first place (I assume there’s some option to make it read the image and add width and height with the correct values, because it was written before CSS scale() existed)

@terencehonles
Copy link
Contributor

terencehonles commented Apr 3, 2021

We have to make GitHub parametrize its use of HTML::Pipeline::SanitizationFilter to allow some specific styles.

Would that be too hard to do? It seems like it must be done in order for the tests to pass. That does point out that it might still end up being stripped by GitHub in their prod pipeline, but as stated in html-pipeline GitHub uses a similar pattern, but not that gem.

This project was started at GitHub. While GitHub still uses a similar design and pattern for rendering content, this gem should be considered standalone and independent from GitHub.

So either way they need to be aware they should not be stripping the inline styles.

@terencehonles
Copy link
Contributor

From this on the README:

  1. The HTML is sanitized, aggressively removing things that could harm you and your kin—such as script tags, inline-styles, and class or id attributes.

this fix may never work unless they do allow some inline styles.

@flying-sheep
Copy link
Contributor Author

Pretty much only this one. width and height are OK as attributes, but align is deprecated in favor of style="float: …".

But we need someone from GitHub providing transparency here.

Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Nov 30, 2024
@flying-sheep
Copy link
Contributor Author

Not my fault that there has been no recent activity

@github-actions github-actions bot removed the Stale label Dec 1, 2024
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.

Image alignment not respected in reStructuredText
5 participants