Fix broken tables whose msgstr is a specific type of HTML #253
Conversation
If an msgstr for a table cell contains HTML that could be parsed as an HTML block if it were put in the top-level, the table is rendered incorrectly. The table row containing the cell gets interrupted in the middle and split into two (or more, if there are more such cells) incomplete rows. Close google#251.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #253 +/- ##
==========================================
+ Coverage 84.74% 85.37% +0.62%
==========================================
Files 15 16 +1
Lines 3377 3527 +150
Branches 3377 3527 +150
==========================================
+ Hits 2862 3011 +149
+ Misses 413 412 -1
- Partials 102 104 +2 ☔ View full report in Codecov by Sentry. |
i18n-helpers/src/lib.rs
Outdated
| Event::SoftBreak => Event::Text(" ".into()), | ||
| // Shortcut links like "[foo]" end up as "[foo]" | ||
| // in output. By changing them to a reference | ||
| // link, the link is expanded on the fly and the | ||
| // output becomes self-contained. | ||
| Event::Start(tag @ (Tag::Link { .. } | Tag::Image { .. })) => { |
There was a problem hiding this comment.
This is duplicating some existing functionality, right? I feel I remember seeing this before 😄
If so, we should pull this filtering code out into its own function so that we can call it from here as well.
There was a problem hiding this comment.
Right, some parts are copied from the _ => arm below. I'll try to making such function.
| @@ -137,6 +140,33 @@ pub fn extract_events<'a>(text: &'a str, state: Option<State<'a>>) -> Vec<(usize | |||
| .enumerate() | |||
| .map(|(idx, line)| (idx + 1, Event::Text(line.into()))) | |||
| .collect(), | |||
| // If we're in a table cell, we put the text in a minimal table, parse the | |||
There was a problem hiding this comment.
A question about the minimal table: what happens if the text contains a | character?
I am not 100% sure what the Commonmark rules are for escaping pipes in tables — can you add another test to document the behavior if it's supported? If it's not supported, I would appreciate a comment here saying that we're sure text doensn't contain | because we know we're in a table context.
There was a problem hiding this comment.
I would say it's basically supported, so I'll add a test for that.
Technically, I can't say text doesn't contain |, because translators could write arbitrary msgstrs in .po... As long as they don't introduce any non-escaped |s, it works fine.
i18n-helpers/src/gettext.rs
Outdated
| "\ | ||
| | Icon | Description |\n\ | ||
| |-------------------------------------------------|-------------------|\n\ | ||
| | <img src=\"some-icon.svg\" alt=\"Some icon.\"/> | Some description. |", |
There was a problem hiding this comment.
Just an idea: consider "translating the src attribute and dropping the alt attribute to make the example smaller. I believe one attribute should be enough to demonstrate this.
There was a problem hiding this comment.
Sounds good! I just intended to write some practical example, but it isn't really necessary for testing.
|
Hi @shinmili, thank you so much for putting this up! I think this looks good and I left some minor comments. |
If an msgstr for a table cell contains HTML that could be parsed as an HTML block if it were put in the top-level, the table is rendered incorrectly. The table row containing the cell gets interrupted in the middle and split into two (or more, if there are more such cells) incomplete rows.
Close #251.