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

Fixes for Issue #2857: Text Styling Should Not Be Applied In The Middle Of URLs #2918

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
4 changes: 4 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@
- Remove the `converse-carbons` plugin and make carbons part of the `converse-chat` plugin.
- Remove the `message_carbons` configuration setting. Carbons are now always enabled.

## 9.1.2 (2022-05-15)
- Added fixes for Issue #2857. The fix is regarding Converse.js incorrectly applying text styling effects to HTTP URLs that contain special characters(`_`, `\``, `\`\`\``, `~`, `*`). With the fixes applied, the hyperlinks encompass the entire HTTP URL instead of only part of it
- Added integration test for the fixes above to the following file: `/src/plugins/chatview/tests/chatbox.js/`

## 9.1.1 (2022-05-05)

- GIFs don't render inside unfurls and cause a TypeError
Expand Down
41 changes: 41 additions & 0 deletions src/plugins/chatview/tests/chatbox.js
Original file line number Diff line number Diff line change
Expand Up @@ -892,6 +892,45 @@ describe("Chatboxes", function () {
}));
});
});

describe("Chatbox Message Styling", function() {

it("is not applied when sending a chat message containing a HTTP URL with styling template characters( _, \`, \`\`\`, ~, *) in the middle of it",
mock.initConverse(['chatBoxesFetched'], {}, async function (_converse) {

await mock.waitForRoster(_converse, 'current');
await mock.openControlBox(_converse);
const contact_jid = mock.cur_names[0].replace(/ /g,'.').toLowerCase() + '@montague.lit';

spyOn(_converse.api, "trigger").and.callThrough();
await mock.openChatBoxFor(_converse, contact_jid);
const view = _converse.chatboxviews.get(contact_jid);
let message = 'https://nitter.kavin.rocks/_MG_/~/*/1506109152665382920';
await mock.sendMessage(view, message);

expect(view.model.messages.length === 1).toBeTruthy();
const stored_messages = await view.model.messages.browserStorage.findAll();
expect(view.model.messages.models[0].attributes.message).toEqual(message);
expect(view.model.messages.length).toBe(1);

// try second URL
let message2 = 'https://nitter.george.rocks/_G*_/\`/\`\`\`/665382920';
await mock.sendMessage(view, message2);

expect(view.model.messages.length === 2).toBeTruthy();
expect(view.model.messages.models[1].attributes.message).toEqual(message2);
expect(view.model.messages.length).toBe(2);

// try third URL
let message3 = 'https://nitter.frank.rocks/_OP_/_NP_/\`\`\`/*/~/_qweq_665382920';
await mock.sendMessage(view, message3);

expect(view.model.messages.length === 3).toBeTruthy();
expect(view.model.messages.models[2].attributes.message).toEqual(message3);
expect(view.model.messages.length).toBe(3);
}));

});;
});

describe("Special Messages", function () {
Expand Down Expand Up @@ -1052,5 +1091,7 @@ describe("Chatboxes", function () {
await mock.openChatBoxFor(_converse, sender_jid);
expect(select_msgs_indicator().textContent).toBe('1');
}));


});
});
19 changes: 18 additions & 1 deletion src/shared/rich-text.js
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,13 @@ export class RichText extends String {
references.forEach(ref => this.addTemplateResult(ref.begin, ref.end, ref.template));
}

checkIfHttpsUrl (text) {
if(text.slice(0, 8) === "https://"){
return true;
}
return false;
}

trimMeMessage () {
if (this.offset === 0) {
// Subtract `/me ` from 3rd person messages
Expand Down Expand Up @@ -300,7 +307,15 @@ export class RichText extends String {
*/
await api.trigger('beforeMessageBodyTransformed', this, { 'Synchronous': true });

this.render_styling && this.addStyling();
var text = this.toString();

if(!this.checkIfHttpsUrl(text)){
this.render_styling && this.addStyling();
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for taking the time and making this pull request.

The above fix isn't sufficient because the text is not guaranteed to be only a URL. There could be URLs anywhere in the text, not just at the start.

So for example AFAICT your fix wouldn't work on this text:

Here is the link I wanted to send you: https://nitter.kavin.rocks/_MG_/status/1506109152665382920

It's great that you're adding tests, so now you can add a test for the message above, check whether it passes and if it doesn't you can improve the code until it does.

For a better solution, take a look at how @ mentions are ignored when looking for styled text.
See here:

const mention_ranges = this.mentions.map(m =>

Similar to how the mention_ranges array is used to avoid styling mentions, we need to know where the URLs are so that we can avoid them as well.

Looks like the urls_meta array has the necessary information (similarly to mention_ranges).
See here:

const urls_meta = this.media_urls || getMediaURLsMetadata(text, local_offset).media_urls || [];

So, in addStyling, you can compute urls_meta and then use that to avoid styling URLs, similarly to how we avoid styling mentions.

You can probably even combine both URLs and mentions into one array of ranges of text which shouldn't be styled.

Copy link
Author

Choose a reason for hiding this comment

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

Ok I see what you mean.
Do you happen to know where this.model.get('references') is being populated?
I ask because this.mentions is populated by this.model.get('references') as seen in src/shared/chat/message-body.js line 48 but I'm not finding where the references field is being populated...

If I can find out how references are being identified, I can make a similar implementation for HTTP Urls

Copy link
Member

@jcbrand jcbrand Jun 10, 2022

Choose a reason for hiding this comment

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

For incoming messages, the references are extracted when the stanza is parsed:

'references': getReferences(stanza),

For outgoing messages it's done somewhere else, I'm not sure where right now. However, I don't think this is relevant to fixing this bug.

The getMediaURLsMetadata function does the work for you of getting the URLs and you can call that function inside addStyling in order to get a list of URLs to ignore.

}else{
this.render_styling;
}

// this.render_styling && this.addStyling();
this.addAnnotations(this.addMentions);
this.addAnnotations(this.addHyperlinks);
this.addAnnotations(this.addMapURLs);
Expand Down Expand Up @@ -342,6 +357,8 @@ export class RichText extends String {
this.references.push({ begin, end, template });
}



isMeCommand () {
const text = this.toString();
if (!text) {
Expand Down