Skip to content
This repository has been archived by the owner on Sep 27, 2024. It is now read-only.

Replace paragraphs with line breaks in message output #834

Merged
merged 28 commits into from
Oct 18, 2023
Merged

Replace paragraphs with line breaks in message output #834

Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
96a182d
Replace paragraphs with line breaks in message output
jonnyandrew Oct 2, 2023
1562880
Parse line breaks to paragraphs
jonnyandrew Oct 2, 2023
07aa46a
Update rendering tests
jonnyandrew Oct 3, 2023
d7964e5
Fix deletion tests
jonnyandrew Oct 4, 2023
536a4b4
Update find range tests
jonnyandrew Oct 4, 2023
15f4166
Update inline code tests
jonnyandrew Oct 4, 2023
983a436
Update example format tests
jonnyandrew Oct 4, 2023
0268f47
Update paragraphs tests
jonnyandrew Oct 4, 2023
e6537e3
Update mentions tests
jonnyandrew Oct 4, 2023
efd2aed
Update quotes tests
jonnyandrew Oct 4, 2023
b5310c1
Update wrap in block tests
jonnyandrew Oct 4, 2023
a0bd138
Update wrap in list tests
jonnyandrew Oct 4, 2023
519bac4
Update dom iterator tests
jonnyandrew Oct 4, 2023
ef5faa0
Update characters test
jonnyandrew Oct 4, 2023
ac09d04
Update formatting tests
jonnyandrew Oct 4, 2023
da49ec5
Update link tests
jonnyandrew Oct 4, 2023
519ff31
Refactor
jonnyandrew Oct 4, 2023
74337c9
Update web test
jonnyandrew Oct 4, 2023
9539239
Handle paragraphs mixed with inline content
jonnyandrew Oct 5, 2023
a40c98a
Fix repeated post-processing by web parser
jonnyandrew Oct 5, 2023
e6370a3
Don't add line breaks where it is implicit
jonnyandrew Oct 5, 2023
a642b37
Fix clippy issue
jonnyandrew Oct 5, 2023
482862b
Add documentation to explain paragaph grouping logic
jonnyandrew Oct 11, 2023
76fa0b7
Merge branch 'main' of github.com:matrix-org/matrix-rich-text-editor …
jonnyandrew Oct 12, 2023
9ed1a4f
update snapshot on ios
langleyd Oct 12, 2023
e3ed1f6
Revert "update snapshot on ios"
langleyd Oct 12, 2023
ba2e8c9
Update SnapshotTests+Blocks.swift
langleyd Oct 12, 2023
88e69e5
Fix testMultipleBlocksContent according to new format.
langleyd Oct 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions crates/wysiwyg/src/composer_model/delete_text.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,18 +224,12 @@ where
};
}
match self.state.dom.lookup_node_mut(&location.node_handle) {
// we should never be passed a container
DomNode::Container(_) => ComposerUpdate::keep(),
DomNode::LineBreak(_) => {
// for a linebreak, remove it if we started the operation from the whitespace
// char type, otherwise keep it
match start_type {
CharType::Whitespace => self.delete_to_cursor(
direction.increment(location.index_in_dom()),
),
_ => ComposerUpdate::keep(),
}
}
DomNode::Container(_) | DomNode::LineBreak(_) => match start_type {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we convert paragraphs to line breaks do we still need LineBreak type internally in the Dom?

Copy link
Contributor Author

@jonnyandrew jonnyandrew Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The function to add line breaks is deprecated and fell out of use at the point of doing #472 I believe, so the Dom shouldn't contain these nodes. However, the LineBreak node is still being used to represent line breaks in the Dom immediately after parsing, before being post-processed into paragraphs.

This is a bit problematic because although we can expect that the Dom should no longer contain LineBreaks, there is a lot of existing code that expects that it might which is effectively dead code and a maintenance burden.

Perhaps we can either

  1. Remove the LineBreak node and parse <br> directly to Container. It will be the most effort but, after this PR, we have compatible tests to make this easier. There is also the question of whether this would put too much logic in the HTML parser.
  2. Keep the LineBreak node but refactor it to something like Placeholder(LineBreak), adding an invariant assertion that it should not exist in the Dom. We could remove any logic that handles this node type and replace it with an unreachable! error.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for that. I'm ok with either option, was more trying to understand it's current use.

Copy link
Contributor Author

@jonnyandrew jonnyandrew Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

CharType::Whitespace => self.delete_to_cursor(
direction.increment(location.index_in_dom()),
),
_ => ComposerUpdate::keep(),
},
DomNode::Mention(_) => self
.delete_to_cursor(direction.increment(location.index_in_dom())),
DomNode::Text(node) => {
Expand Down Expand Up @@ -354,7 +348,13 @@ where
) -> Option<CharType> {
let node = self.state.dom.lookup_node(&location.node_handle);
match node {
DomNode::Container(_) => None,
DomNode::Container(node) => {
if node.is_empty() {
Some(CharType::Whitespace)
} else {
None
}
}
DomNode::LineBreak(_) => {
// we have to treat linebreaks as chars, this type fits best
Some(CharType::Whitespace)
Expand Down
149 changes: 110 additions & 39 deletions crates/wysiwyg/src/composer_model/example_format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ impl ComposerModel<Utf16String> {
root.fmt_html(
&mut buf,
Some(&mut selection_writer),
ToHtmlState::default(),
&ToHtmlState::default(),
false,
);
if range.is_empty().not() {
Expand Down Expand Up @@ -591,24 +591,95 @@ mod test {

#[test]
fn cm_creates_correct_component_model_newlines() {
let t0 = cm("|<br />");
assert_eq!(t0.state.start, 0);
assert_eq!(t0.state.end, 0);
assert_eq!(t0.get_content_as_html(), utf16("<br />"));
// TODO: There should only be one node for the br tag
//assert_eq!(t0.state.dom.children().len(), 1);

let t1 = cm("<br />|<br />");
assert_eq!(t1.state.start, 1);
assert_eq!(t1.state.end, 1);
assert_eq!(t1.get_content_as_html(), utf16("<br /><br />"));
// TODO: assert_eq!(t1.state.dom.children().len(), 2);

let t2 = cm("<br /><br />|");
assert_eq!(t2.state.start, 2);
assert_eq!(t2.state.end, 2);
assert_eq!(t2.get_content_as_html(), utf16("<br /><br />"));
// TODO: assert_eq!(t1.state.dom.children().len(), 2);
assert_eq!(
cm("<br />|").get_content_as_html(),
"<p>\u{a0}</p><p>\u{a0}</p>"
);
assert_eq!(
cm("<br /><br />|").get_content_as_html(),
"<p>\u{a0}</p><p>\u{a0}</p><p>\u{a0}</p>"
);
assert_eq!(
cm("<br />|<br />").get_content_as_html(),
"<p>\u{a0}</p><p>\u{a0}</p><p>\u{a0}</p>"
);
assert_eq!(cm("a<br />b|").get_content_as_html(), "<p>a</p><p>b</p>");
assert_eq!(
cm("a<br />|<br />b").get_content_as_html(),
"<p>a</p><p>\u{a0}</p><p>b</p>"
);
assert_eq!(
cm("a<br />b|<br />c").get_content_as_html(),
"<p>a</p><p>b</p><p>c</p>"
);
assert_eq!(
cm("a<br />|b<br />c").get_content_as_html(),
"<p>a</p><p>b</p><p>c</p>"
);
assert_eq!(
cm("<b>a<br />|b<br />c</b>").get_content_as_html(),
"<p><b>a</b></p><p><b>b</b></p><p><b>c</b></p>"
);
assert_eq!(
cm("|<br />").get_content_as_html(),
"<p>\u{a0}</p><p>\u{a0}</p>"
);
assert_eq!(
cm("aaa<br />|bbb").get_content_as_html(),
"<p>aaa</p><p>bbb</p>"
);
assert_eq!(
cm("aaa|<br />bbb").get_content_as_html(),
"<p>aaa</p><p>bbb</p>"
);
assert_eq!(
cm("aa{a<br />b}|bb").get_content_as_html(),
"<p>aaa</p><p>bbb</p>"
);
assert_eq!(cm("aa{a<br />b}|bb").state.start, 2);
assert_eq!(cm("aa{a<br />b}|bb").state.end, 5);
assert_eq!(
cm("aa|{a<br />b}bb").get_content_as_html(),
"<p>aaa</p><p>bbb</p>"
);
assert_eq!(cm("aa|{a<br />b}bb").state.start, 5);
assert_eq!(cm("aa|{a<br />b}bb").state.end, 2);
assert_eq!(
cm("aa{<br />b}|bb").get_content_as_html(),
"<p>aa</p><p>bbb</p>"
);
assert_eq!(cm("aa{<br />b}|bb").state.start, 2);
assert_eq!(cm("aa{<br />b}|bb").state.end, 4);
assert_eq!(
cm("aa|{<br />b}bb").get_content_as_html(),
"<p>aa</p><p>bbb</p>"
);
assert_eq!(cm("aa|{<br />b}bb").state.start, 4);
assert_eq!(cm("aa|{<br />b}bb").state.end, 2);
assert_eq!(
cm("aa{a<br />b}|bb").get_content_as_html(),
"<p>aaa</p><p>bbb</p>"
);
assert_eq!(cm("aa{a<br />b}|bb").state.start, 2);
assert_eq!(cm("aa{a<br />b}|bb").state.end, 5);
assert_eq!(
cm("aa|{a<br />}bb").get_content_as_html(),
"<p>aaa</p><p>bb</p>"
);
assert_eq!(cm("aa|{a<br />}bb").state.start, 4);
assert_eq!(cm("aa|{a<br />}bb").state.end, 2);
assert_eq!(
cm("aa{<br />}|bb").get_content_as_html(),
"<p>aa</p><p>bb</p>"
);
assert_eq!(cm("aa{<br />}|bb").state.start, 2);
assert_eq!(cm("aa{<br />}|bb").state.end, 3);
assert_eq!(
cm("aa|{<br />}bb").get_content_as_html(),
"<p>aa</p><p>bb</p>"
);
assert_eq!(cm("aa|{<br />}bb").state.start, 3);
assert_eq!(cm("aa|{<br />}bb").state.end, 2);
}

#[test]
Expand Down Expand Up @@ -814,27 +885,27 @@ mod test {
assert_that!("AAA<b>B{BB</b>C}|CC").roundtrips();
assert_that!("AAA<b>B|{BB</b>C}CC").roundtrips();
assert_that!("<ul><li>~|</li></ul>").roundtrips();
assert_that!("<br />|").roundtrips();
assert_that!("<br /><br />|").roundtrips();
assert_that!("<br />|<br />").roundtrips();
assert_that!("<br />|<br />").roundtrips();
assert_that!("a<br />b|").roundtrips();
assert_that!("a<br />|<br />b").roundtrips();
assert_that!("a<br />b|<br />c").roundtrips();
assert_that!("a<br />|b<br />c").roundtrips();
assert_that!("<b>a<br />|b<br />c</b>").roundtrips();
assert_that!("|<br />").roundtrips();
assert_that!("aaa<br />|bbb").roundtrips();
assert_that!("aaa|<br />bbb").roundtrips();
assert_that!("aa{a<br />b}|bb").roundtrips();
assert_that!("aa|{a<br />b}bb").roundtrips();
assert_that!("aa{<br />b}|bb").roundtrips();
assert_that!("aa|{<br />b}bb").roundtrips();
assert_that!("aa{a<br />b}|bb").roundtrips();
assert_that!("aa|{a<br />}bb").roundtrips();
assert_that!("aa{<br />}|bb").roundtrips();
assert_that!("aa|{<br />}bb").roundtrips();
assert_that!("<ol><li>a|</li></ol>").roundtrips();
assert_that!("<p>aa{a</p><p>b}|bb</p>").roundtrips();
assert_that!("<p>aa|{a}</p><p>bb</p>").roundtrips();
assert_that!("<p> </p><p> |</p>").roundtrips();
assert_that!("<p> </p><p> |</p><p> </p>").roundtrips();
assert_that!("<p>a</p><p>b|</p>").roundtrips();
assert_that!("<p>a</p><p>|b</p>").roundtrips();
assert_that!("<p>a</p><p>b|</p><p>c</p>").roundtrips();
assert_that!("<p>a</p><p>|b</p><p>c</p>").roundtrips();
assert_that!("<p><b>a</b></p><p><b>|b</b></p><p><b>c</b></p>")
.roundtrips();
assert_that!("<p> |</p><p> </p>").roundtrips();
assert_that!("<p>aaa</p><p>|bbb</p>").roundtrips();
assert_that!("<p>aaa|</p><p>bbb</p>").roundtrips();
assert_that!("<p>aa{a</p><p>b}|bb</p>").roundtrips();
assert_that!("<p>aa|{a</p><p>b}bb</p>").roundtrips();
assert_that!("<p>aa</p><p>{b}|bb</p>").roundtrips();
assert_that!("<p>aa</p><p>|{b}bb</p>").roundtrips();
assert_that!("<p>aa|{a}</p><p>bb</p>").roundtrips();
assert_that!("<p>aa</p><p>|bb</p>").roundtrips();
assert_that!("<p>aa|</p><p>bb</p>").roundtrips();
}

#[test]
Expand Down
12 changes: 8 additions & 4 deletions crates/wysiwyg/src/composer_model/format_inline_code.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,10 @@ mod test {
fn inline_code_with_formatting_preserves_line_breaks() {
let mut model = cm("<b>{bold</b><br /><i>text}|</i>");
model.inline_code();
assert_eq!(tx(&model), "<code>{bold<br />text}|</code>");
assert_eq!(
tx(&model),
"<p><code>{bold</code></p><p><code>text}|</code></p>"
);
}

#[test]
Expand Down Expand Up @@ -249,7 +252,8 @@ mod test {
model.inline_code();
assert_eq!(
tx(&model),
"<b><u>bo</u></b><code>{ld<br />te}|</code><i>xt</i>"
"<p><b><u>bo</u></b><code>{ld</code></p><p><code>te}|</code><i>xt</i></p>",

);
}

Expand All @@ -260,7 +264,7 @@ mod test {
model.inline_code();
assert_eq!(
tx(&model),
"<b><u>bo</u></b><code>{ld<br />te}|</code><i>xt</i>"
"<p><b><u>bo</u></b><code>{ld</code></p><p><code>te}|</code><i>xt</i></p>",
);
}

Expand Down Expand Up @@ -328,7 +332,7 @@ mod test {
fn unformat_inline_code_same_row_with_line_breaks() {
let mut model = cm("<code>{bold<br />text}|</code>");
model.inline_code();
assert_eq!(tx(&model), "{bold<br />text}|");
assert_eq!(tx(&model), "<p>{bold</p><p>text}|</p>");
}

#[test]
Expand Down
5 changes: 4 additions & 1 deletion crates/wysiwyg/src/composer_model/new_lines.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,10 @@ where
Generic => {
self.do_new_line_in_paragraph(first_leaf, block_location);
}
_ => panic!("Unexpected kind block node with inline contents"),
_ => panic!(
"Unexpected kind {:?} with inline contents",
block_location.kind
),
}
self.create_update_replace_all()
}
Expand Down
4 changes: 2 additions & 2 deletions crates/wysiwyg/src/composer_model/quotes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ mod test {
model.quote();
assert_eq!(
tx(&model),
"<blockquote><p><b>Some |text</b></p></blockquote><b><br />Next line</b>"
"<blockquote><p><b>Some |text</b></p></blockquote><p><b>Next line</b></p>"
)
}

Expand All @@ -263,7 +263,7 @@ mod test {
model.quote();
assert_eq!(
tx(&model),
"<blockquote><ul><li><b>Some {text<br />Next line</b></li><li><i>Second}| item</i></li></ul></blockquote><ul><li>Third item</li></ul>"
"<blockquote><ul><li><p><b>Some {text</b></p><p><b>Next line</b></p></li><li><i>Second}| item</i></li></ul></blockquote><ul><li>Third item</li></ul>"
)
}

Expand Down
21 changes: 12 additions & 9 deletions crates/wysiwyg/src/dom/dom_block_nodes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,19 +208,21 @@ mod test {
let model = cm("Some text| <b>and bold </b><br/><i>and italic</i>");
let (s, e) = model.safe_selection();
let ret = model.state.dom.find_nodes_to_wrap_in_block(s, e).unwrap();
assert_eq!(ret.ancestor_handle, DomHandle::from_raw(Vec::new()));
assert_eq!(ret.start_handle, DomHandle::from_raw(vec![0]));
assert_eq!(ret.end_handle, DomHandle::from_raw(vec![1, 0]));
// <br> has been converted to <p> hence the extra depth
assert_eq!(ret.ancestor_handle, DomHandle::from_raw(vec![]));
assert_eq!(ret.start_handle, DomHandle::from_raw(vec![0, 0]));
assert_eq!(ret.end_handle, DomHandle::from_raw(vec![0, 1, 0]));
}

#[test]
fn find_ranges_to_wrap_several_nodes_with_line_break_at_start() {
let model = cm("Some text <br/><b>and bold </b><i>|and italic</i>");
let (s, e) = model.safe_selection();
let ret = model.state.dom.find_nodes_to_wrap_in_block(s, e).unwrap();
assert_eq!(ret.ancestor_handle, DomHandle::from_raw(Vec::new()));
assert_eq!(ret.start_handle, DomHandle::from_raw(vec![2, 0]));
assert_eq!(ret.end_handle, DomHandle::from_raw(vec![3, 0]));
// <br> has been converted to <p> hence the extra depth
assert_eq!(ret.ancestor_handle, DomHandle::from_raw(vec![]));
assert_eq!(ret.start_handle, DomHandle::from_raw(vec![1, 0, 0]));
assert_eq!(ret.end_handle, DomHandle::from_raw(vec![1, 1, 0]));
}

#[test]
Expand All @@ -242,9 +244,10 @@ mod test {
);
let (s, e) = model.safe_selection();
let ret = model.state.dom.find_nodes_to_wrap_in_block(s, e).unwrap();
assert_eq!(ret.ancestor_handle, DomHandle::from_raw(vec![0, 0]));
assert_eq!(ret.start_handle, DomHandle::from_raw(vec![0, 0, 3, 0]));
assert_eq!(ret.end_handle, DomHandle::from_raw(vec![0, 0, 3, 0]));
// <br> has been converted to <p> hence the extra depth
assert_eq!(ret.ancestor_handle, DomHandle::from_raw(vec![0, 0, 1]));
assert_eq!(ret.start_handle, DomHandle::from_raw(vec![0, 0, 1, 0, 0]));
assert_eq!(ret.end_handle, DomHandle::from_raw(vec![0, 0, 1, 0, 0]));
}

#[test]
Expand Down
Loading
Loading