-
Notifications
You must be signed in to change notification settings - Fork 23
Amend mention message html output #716
Changes from all commits
abe9789
5d9183e
359fb2d
0f05b67
4f84427
c2f3285
cf47a8c
802492f
fa8fd09
0fab533
50a450c
e8bd655
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,7 +77,16 @@ where | |
let (start, end) = self.safe_selection(); | ||
let range = self.state.dom.find_range(start, end); | ||
|
||
let new_node = DomNode::new_mention(url, text, attributes); | ||
// use the display text decide the mention type | ||
// TODO extract this into a util function if it is reused when parsing the html prior to editing a message | ||
// TODO decide if this do* function should be separated to handle mention vs at-room mention | ||
// TODO handle invalid mention urls after permalink parsing methods have been created | ||
let new_node = if text == "@room".into() { | ||
DomNode::new_at_room_mention(attributes) | ||
} else { | ||
DomNode::new_mention(url, text, attributes) | ||
Comment on lines
+86
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When trying to insert an invalid mention, should the editor still do that? I'm wondering if it could return an error result and/or fall back to It might not be possible to do without being able to parse mentions but could be worth adding a note There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good question, and for now I believe we don't have a way of telling if it's valid or not. Will add a note to handle that (after parsing gives us that ability). |
||
}; | ||
|
||
let new_cursor_index = start + new_node.text_len(); | ||
|
||
let handle = self.state.dom.insert_node_at_cursor(&range, new_node); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -129,22 +129,34 @@ impl<S: UnicodeString> MentionNode<S> { | |
let cur_pos = formatter.len(); | ||
match self.kind() { | ||
MentionNodeKind::MatrixUrl { display_text, url } => { | ||
let mut attributes = self.attributes.clone(); | ||
attributes.push(("href".into(), url.clone())); | ||
|
||
if !as_message { | ||
attributes.push(("contenteditable".into(), "false".into())) | ||
// TODO: data-mention-type = "user" | "room" | ||
} | ||
// if formatting as a message, only include the href attribute | ||
let attributes = if as_message { | ||
vec![("href".into(), url.clone())] | ||
} else { | ||
let mut attributes_for_composer = self.attributes.clone(); | ||
attributes_for_composer.push(("href".into(), url.clone())); | ||
attributes_for_composer | ||
.push(("contenteditable".into(), "false".into())); | ||
attributes_for_composer | ||
}; | ||
|
||
self.fmt_tag_open(tag, formatter, &Some(attributes)); | ||
|
||
formatter.push(display_text.clone()); | ||
|
||
self.fmt_tag_close(tag, formatter); | ||
} | ||
MentionNodeKind::AtRoom => { | ||
formatter.push(self.display_text()); | ||
// if formatting as a message, simply use the display text (@room) | ||
if as_message { | ||
formatter.push(self.display_text()) | ||
} else { | ||
let mut attributes = self.attributes.clone(); | ||
attributes.push(("href".into(), "#".into())); // designates a placeholder link in html | ||
attributes.push(("contenteditable".into(), "false".into())); | ||
|
||
self.fmt_tag_open(tag, formatter, &Some(attributes)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this tag be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the time being keeping everything as |
||
formatter.push(self.display_text()); | ||
self.fmt_tag_close(tag, formatter); | ||
}; | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
// Copyright 2023 The Matrix.org Foundation C.I.C. | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
use crate::tests::testutils_composer_model::{cm, tx}; | ||
|
||
#[test] | ||
fn replaces_empty_paragraphs_with_newline_characters() { | ||
let mut model = cm("|"); | ||
model.replace_text("hello".into()); | ||
model.enter(); | ||
model.enter(); | ||
model.enter(); | ||
model.enter(); | ||
model.replace_text("Alice".into()); | ||
|
||
assert_eq!( | ||
tx(&model), | ||
"<p>hello</p><p> </p><p> </p><p> </p><p>Alice|</p>" | ||
); | ||
let message_output = model.get_content_as_message_html(); | ||
assert_eq!(message_output, "<p>hello</p>\n\n\n<p>Alice</p>"); | ||
} | ||
#[test] | ||
fn only_outputs_href_attribute_on_user_mention() { | ||
let mut model = cm("|"); | ||
model.insert_mention( | ||
"www.url.com".into(), | ||
"inner text".into(), | ||
vec![ | ||
("data-mention-type".into(), "user".into()), | ||
("style".into(), "some css".into()), | ||
], | ||
); | ||
assert_eq!(tx(&model), "<a data-mention-type=\"user\" style=\"some css\" href=\"www.url.com\" contenteditable=\"false\">inner text</a> |"); | ||
|
||
let message_output = model.get_content_as_message_html(); | ||
assert_eq!( | ||
message_output, | ||
"<a href=\"www.url.com\">inner text</a>\u{a0}" | ||
); | ||
} | ||
|
||
#[test] | ||
fn only_outputs_href_attribute_on_room_mention() { | ||
let mut model = cm("|"); | ||
model.insert_mention( | ||
"www.url.com".into(), | ||
"inner text".into(), | ||
vec![ | ||
("data-mention-type".into(), "room".into()), | ||
("style".into(), "some css".into()), | ||
], | ||
); | ||
assert_eq!(tx(&model), "<a data-mention-type=\"room\" style=\"some css\" href=\"www.url.com\" contenteditable=\"false\">inner text</a> |"); | ||
|
||
let message_output = model.get_content_as_message_html(); | ||
assert_eq!( | ||
message_output, | ||
"<a href=\"www.url.com\">inner text</a>\u{a0}" | ||
); | ||
} | ||
|
||
#[test] | ||
fn only_outputs_href_inner_text_for_at_room_mention() { | ||
let mut model = cm("|"); | ||
model.insert_mention( | ||
"anything".into(), // this should be ignored in favour of a # placeholder | ||
"@room".into(), | ||
vec![ | ||
("data-mention-type".into(), "at-room".into()), | ||
("style".into(), "some css".into()), | ||
], | ||
); | ||
assert_eq!(tx(&model), "<a data-mention-type=\"at-room\" style=\"some css\" href=\"#\" contenteditable=\"false\">@room</a> |"); | ||
|
||
let message_output = model.get_content_as_message_html(); | ||
assert_eq!(message_output, "@room\u{a0}"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this function is able to create an
@room
mention, we might want to reconsider the API, and either:url
argument optional, orAtRoom
|Matrix(text, url)
- the actual types might come from Add Matrix mentions utils [wip] #695).Alternatively, we could add a new function so that we have:
do_insert_mention(text, url) // with mandatory args
do_insert_at_room_mention()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd be tempted to go a different way and have this function call
DomNode::new_mention
with url, display_text, attributes as we currently have and then haveMentionNode::new
create either a room, user, or at-room mention as appropriate.What do you think to that solution?
If you're happy with me adding a note as the work may be further informed by the WIP mentions utils work (which is what I'm picking up next), I'll do that and address in that subsequent PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes sense. I still think the API could be more self explanatory in terms of URL being optional, and some documentation of that behaviour if we do keep a single function for both. But happy for that to be addressed in a later PR