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

AO3-6464 Allow ruby, rt, and rp HTML tags where HTML is allowed #4443

Merged
merged 4 commits into from
Feb 20, 2023

Conversation

potpotkettle
Copy link
Contributor

Pull Request Checklist

Issue

https://otwarchive.atlassian.net/browse/AO3-6464

Purpose

The ruby, rt, and rp tags are used to represent ruby annotation. With these tags, the small text marked with rt will be located above the base text marked with ruby. Although there are various complexities that come with more advanced features of ruby annotation (e.g. the tabular syntax, CSS rules to control how to render ruby characters, etc), the intention behind the linked Jira issue seems to be to support the most simple (and common) use cases.

Testing Instructions

To start with, please try posting a new work with the new tags (and with the usual required fields).

Suggested content: <ruby>BigText<rp> (</rp><rt>short_text</rt><rp>)</rp></ruby>

This should work in other places where the limited HTML is allowed (like comments).

References

rb is not included here (thus removed by the sanitizer when entered). I think this could potentially be revisited if and when whatwg/html#1771 is resolved. See also the linked Jira issue for related discussion.

Credit

Potpotkettle (they/them)

@potpotkettle
Copy link
Contributor Author

Here is a current screenshot.
Screenshot 2023-01-28 at 12-39-22 (development) Work with annotations - pot - Original work Example Archive

I have also attached downloaded versions in PDF, AZW3 and MOBI. When viewed from an app called Foliate, the mobi version was rendered with parentheses. The rest were the same as the web site version (annotations above the main text).

@@ -4,7 +4,7 @@ <h3>Allowed HTML</h3>
<p>
<code>a, abbr, acronym, address, [align], [alt], [axis], b, big, blockquote, br, caption, center, cite, [class], code,
col, colgroup, dd, del, dfn, div, dl, dt, em, figcaption, figure, h1, h2, h3, h4, h5, h6, [height], hr, [href], i, img,
ins, kbd, li, [name], ol, p, pre, q, s, samp, small, span, [src], strike, strong, sub, sup, table, tbody, td,
ins, kbd, li, [name], ol, p, pre, q, rp, rt, ruby, s, samp, small, span, [src], strike, strong, sub, sup, table, tbody, td,
Copy link
Member

Choose a reason for hiding this comment

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

There's another list at

def allowed_html_instructions(show_list = false, show_text=true)
(show_text ? h(ts("Plain text with limited HTML")) : ''.html_safe) +
link_to_help("html-help") + (show_list ?
"<code>a, abbr, acronym, address, [alt], [axis], b, big, blockquote, br, caption, center, cite, [class], code,
col, colgroup, dd, del, dfn, [dir], div, dl, dt, em, h1, h2, h3, h4, h5, h6, [height], hr, [href], i, img,
ins, kbd, li, [name], ol, p, pre, q, s, samp, small, span, [src], strike, strong, sub, sup, table, tbody, td,
tfoot, th, thead, [title], tr, tt, u, ul, var, [width]</code>" : "").html_safe
end

but I don't think we ever set show_list = true anywhere. Can you remove that option and the conditional that uses it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For all I could find, it was disabled in 1d9f656 (in 2011).

The option is used in app/views/bookmarks/_bookmark_form.html.erb and app/views/comments/_comment_form.html.erb (with show_list=false). I feel like I would be touching parts of the code that's not particularly related to the issue (ruby annotations), but if that's okay, sure, I'll do that.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to make these changes. It's unused code that looks puzzling if not updated to include to new tags.

@@ -85,4 +85,9 @@
word_counter.text = "\“嘿Bob,\” Alice说,‘啊?!?’"
expect(word_counter.count).to eq(5)
end

it "doesn't count parentheses in rp" do
Copy link
Member

Choose a reason for hiding this comment

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

You can leave out this test, which is not too related to the current issue (the word counter ignores all HTML tags).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true that this is not a test that justifies the other changes in the PR. It would have passed before. So I agree with leaving it out if that's what you mean.

There doesn't seem to be tests that involve parenthesized text, full-width or not, and parentheses might be common things that would warrant tests, but it seems like that's a separate issue to think about.

(That's a long way to say "yes" but I wanted to make sure I have it right.)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's leave this alone for now as it's not related to the main changes in the PR.

Copy link
Member

@redsummernight redsummernight left a comment

Choose a reason for hiding this comment

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

Thank you!

@sarken
Copy link
Collaborator

sarken commented Feb 20, 2023

Hey, @potpotkettle! Do you have a few minutes to resolve the merge conflicts on this? If not, no worries, someone else can take them.

@potpotkettle
Copy link
Contributor Author

Sure, done.

@redsummernight
Copy link
Member

Thanks!

@redsummernight redsummernight merged commit c374f6b into otwcode:master Feb 20, 2023
@potpotkettle potpotkettle deleted the ruby-rt-rp-AO3-6464 branch July 6, 2023 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants