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

Matrix.to permalinks should be pills #1030

Closed
12 tasks done
daniellekirkwood opened this issue Feb 21, 2023 · 24 comments
Closed
12 tasks done

Matrix.to permalinks should be pills #1030

daniellekirkwood opened this issue Feb 21, 2023 · 24 comments
Assignees
Labels
A-Matrix.to A-Pills A-Timeline T-Enhancement T-Epic Issue is at Epic level Z-AirFocus Moving issues from GH to AirFocus purposefully using this tag. Z-PS-Request

Comments

@daniellekirkwood
Copy link
Contributor

daniellekirkwood commented Feb 21, 2023

this is a duplicate on purpose, we do not want to repurpose the current issue in order to satisfy the customer request

Matrix.to links, when posted in Element timeline, should transform from their URL format to a 'pill'

To satisfy the customer request we're scoping the below behaviour to apply to permalinks only. If a user sends a raw MXID it should not pillify. If the user sends a matrix.to permalink, this should be a pill in the timeline.


Matrix.to links to a room/space

  • When a room's Matrix.to link is pasted into the composer and sent to the timeline, it should not show as the URL but instead the pill.
The pill should show the room/space avatar and name, like this: Screenshot 2023-02-21 at 18 13 11
If this is not possible, because the user does not have access to the room/space details, the pill should show the default link icon and "Room/Space". Like this: image
  • where the user has used markdown send a permalink to the room, the pill overtakes the markdown label
  • where the user has used markdown to send a permalink to a room or space, the markdown label should be shown (hyperlink) instead of the default pill if the user doesn't have access to the room/space details

Matrix.to links to an MXID

  • When a user enters a "@mxid"/@DisplayName, it should transform into a pill. When clicked on, the pill should show the user details
The user's name and profile picture should render in the pill itself. Like this: Screenshot 2023-02-21 at 18 09 52
If this is not possible, the pill should show the default user icon and the mxId : image
  • Verify if this is the case on all 3 platforms today
  • where the user has used markdown send a permalink to the user, the pill overtakes the markdown label
  • where the user has used markdown to send a permalink to a user, the markdown label should be shown (hyperlink) instead of the default pill if the user doesn't have access to the user details
  • If the user pastes an MXID and has not selected the pill, it should show as text (similar to today) see internal discussion

Matrix.to links to a message in the same room

  • When a user has shared a Matrix.to link to a message in the same room,
it should show like this: image
If we failed to retrieve the event or the sender details, just show "Message" as a pill with the link icon image
  • Before committing to this we need to better understand the performance implications in this case, if it's a drag on resources for the app then we should implement the below for all message matrix.to links.
  • where the user has used markdown send a permalink to the message, the markdown label should be shown, not the pill

Matrix.to links to a message in a different room

  • When a user has shared a matrix.to link to a message in another room
it should show like this (with the avatar and room name): image
If these are not available, apply the same logic as above (the pill should show the default room icon and "Room") image
  • where the user has used markdown send a permalink to the message, the markdown label should be shown, not the pill

We're not aiming to change the behaviour of the pills, just polish the feature and extend the functionality to matrix.to links for specific messages. IE: If the composer renders the pill, that should continue to be the case.

We will not do Alias handling or URL 'pillifying' in this round, though we should look to improve those things in the future. We also will not update the share dialog at this time.


Notes

  • We need to understand what would happen if the pill wraps over more than one line
  • We need to understand what state each platform is currently in, and whether we should create fixes, or start from scratch
  • The RTE team have their own issue tracking this version of the work - we will not implement anything in the RTE, instead they will aim to match the behaviour we're implementing. As both composers will likely live alongside each other - this feels like the best approach

Future

  • In the future we need to consider how we pill more items (like MXIDs) however, for this round the T&S complications were such that it falls out of scope for our project. We need to be cautious about what and how we reveal to users on other users' profiles - especially in a community setting.
  • We chose this approach for the connection with markdown and pillifying due to some technical constraints. We should monitor user feedback and revisit this decision in the future if needed.
@daniellekirkwood daniellekirkwood added T-Enhancement A-Timeline A-Matrix.to A-Pills Z-PS-Request Z-AirFocus Moving issues from GH to AirFocus purposefully using this tag. labels Feb 21, 2023
@daniellekirkwood daniellekirkwood self-assigned this Feb 21, 2023
@daniellekirkwood daniellekirkwood added the T-Epic Issue is at Epic level label Feb 21, 2023
@weeman1337
Copy link

This issue will be solved if we pillify all MXIDs on Web: element-hq/element-web#23342

@weeman1337
Copy link

weeman1337 commented Mar 1, 2023

When copying those pills or text containing the them, they should convert to an URL.
Inside the apps they should also behave like a link to be able to copy them.

@weeman1337
Copy link

We need to understand what would happen if the pill wraps over more than one line

On web it will be truncated like this

image

@giomfo
Copy link
Member

giomfo commented Mar 6, 2023

@giomfo
Copy link
Member

giomfo commented Mar 7, 2023

@daniellekirkwood we need to clarify the expectation about the composer in the following point (Matrix.to links to a room/space):

When a room's Matrix.to link is pasted into the composer and sent to the timeline, it should not show as the URL but instead the pill.

Does that mean the composer should display a pill as soon as a permalink is pasted into it?
If yes, I think the pill should be suggested like for the existing user's pills (completion mode). Indeed the end user may want to edit the pasted URL before sending their message. But then, the implementation would be more complex.
FYI I exclude the composer from this task until we clarify this point

@element-hq element-hq deleted a comment from weeman1337 Mar 8, 2023
@daniellekirkwood
Copy link
Contributor Author

Sending.a.Matrix.to.URL.to.timeline.mov

@giomfo this is the behaviour today, and the behaviour I would expect when pasting a link and sharing it. Is this what you had in mind?

This is only for the paste link use case, and not the Composer in general - if you have questions about more composer behaviour lets have a quick chat about those :)

@giomfo
Copy link
Member

giomfo commented Mar 13, 2023

Thanks @daniellekirkwood this is clear now

@giomfo
Copy link
Member

giomfo commented Mar 13, 2023

New question:
What if someone creates a link to a room/user/message with a custom label?
Like for instance by typing markdown like this [ask this person](https://matrix.to/#/@giom:example.com) or using the WYSIWYG editor. Currently the result is a pill linked to a matrix user, where the custom label is ignored:

Example message:
Without link:
If you have questions about pills, ask this person.

Pill for a link with the label „ask this person“:
image


1- We first considered that we should keep the custom label if any, but in that case we will display an hyperlink (instead of a pill)
-> this will break all the user pills automatically generated by the composer. Indeed in case of completion, a formatted body is sent with a custom label: "If you have questions about pills, <a href="https://matrix.to/#/@giom:example.com\">giom</a>"
2- We thought then that we should check whether the custom label is the user/room display name to keep displaying a pill
when they match
-> we will display an hyperlink as soon as a user/room will change their display name

There is not really a solution because we cannot distinguish custom label auto-filled (by the composer) and those filled by the user.
We decided for the moment to keep the existing implementation at least for the user and room permalinks (custom label would be ignored if any). The case of the permalink to a message may be considered differently because there is no case where a label is auto-filled for these permalinks -> we may decide to keep display the custom label if any (I think this is the most useful case to keep label)

@daniellekirkwood daniellekirkwood changed the title Matrix.to links should be pills Matrix.to permalinks should be pills Mar 15, 2023
@daniellekirkwood
Copy link
Contributor Author

daniellekirkwood commented Mar 28, 2023

@daniellekirkwood testing on March 28, 2023

Web: Element Nightly version: 2023032801
Used composer to @ someone ✅
Sent a link to another room ✅
Sent a link to a user in the room ✅
Sent a link to a user not in the room ✅
Sent a link to a message in the room ✅
Sent a link to a message in a different room ✅
Sent a link to a message in a different room that I'm not in (public) ✅
Sent a link to a message in a different room that I'm not in (private) ✅
Sent a link to a space that this room is not in ✅
Sent a link to a space that this room is in ✅
Forwarded a pill from one room to another ✅
Copied a pill from one room to another ✅
Putting a link to a message inside of a url link showed the label text ✅
Putting a link to a user inside of a url did not show the label text ✅
All of the above took me to the right place when clicking on it ✅
How does the pill look when the screen is small, or the text is big ✅

This failed: If the user pastes an MXID and has not selected the pill, it should show as text (similar to today) see internal discussion

Retested and recorded here and it looks great. ✅

Did not test: Sharing a link to a Space I'm not in using markdown - where the markdown should be shown.


iOS: 1.10.7 (20230322213716)
Used composer to @ someone ✅
Sent a link to another room ✅
Sent a link to a user in the room ✅
Sent a link to a user not in the room - can't find a way to share a user profile in order to trigger this.
Sent a link to a message in the room ✅
Sent a link to a message in a different room ✅
Sent a link to a message in a different room that I'm not in (private) ✅
Sent a link to a space that this room is not in - can't find a way to share
Sent a link to a space that this room is in - can't find a way to share
Forwarded a pill from one room to another 🚫 - can't click on the message to get to the forward option
Copied a pill from one room to another 🚫 - can't click on the message to get to the copy option
Putting a link to a message inside of a url link showed the label text ✅
Putting a link to a user inside of a url did not show the label text 🟡 it showed the whole thing, not just the pill
All of the above took me to the right place when clicking on it ✅
How does the pill look when the screen is small, or the text is big 🚫 Didn't wrap/shorten but I'm pretty sure we knew that would happen...

Did not test: Sharing a link to a Space I'm not in using markdown - where the markdown should be shown.

@giomfo
Copy link
Member

giomfo commented Mar 28, 2023

Forwarded a pill from one room to another 🚫 - can't click on the message to get to the forward option
Copied a pill from one room to another 🚫 - can't click on the message to get to the copy option

@daniellekirkwood you have to click next to the message. On my side Forward is working correctly on iOS. In case of Copy, we copy the formatted body -> we send a label with the permalink, iOS is rendering a hyperlink
image

How does the pill look when the screen is small, or the text is big 🚫 Didn't wrap/shorten but I'm pretty sure we knew that would happen...

This last point should be fixed by element-hq/element-ios#7455

@daniellekirkwood
Copy link
Contributor Author

Forwarded a pill from one room to another 🚫 - can't click on the message to get to the forward option
Copied a pill from one room to another 🚫 - can't click on the message to get to the copy option

@daniellekirkwood you have to click next to the message. On my side Forward is working correctly on iOS. In case of Copy, we copy the formatted body -> we send a label with the permalink, iOS is rendering a hyperlink image

Wonderful! Let's consider it passed then :D ✅

How does the pill look when the screen is small, or the text is big 🚫 Didn't wrap/shorten but I'm pretty sure we knew that would happen...

This last point should be fixed by vector-im/element-ios#7455

Yeah - I thought we had a PR open for it, just couldn't find it :D

@HarHarLinks
Copy link

HarHarLinks commented Mar 31, 2023

This issue is not closed yet, however as feedback was already asked in TWIM, I want to note here:

Matrix.to links to a message in a different room

These should only use the generic name & avatar if they are not accessible. I am experiencing them not being shown despite the room (https://matrix.to/#/!zzGPdcyFqxIOJEijoI:matrix.org/$apwByPtYX72BXt1bKPjQEIkcVTJ87_fI1f6GJmSBGIM?via=sosnowkadub.de&via=matrix.org&via=element.io) being previewable, running develop.element.io Element version: 89054a50acb2-react-212977c4ac42-js-5fc6b3ed1711
Olm version: 3.2.12

Edit: ohey now it suddenly fixed itself, it was just real slow previewing that info as I wasn't in the room.


I would also suggest to change the pill from [icon] Message in <room> to Message in [icon] <room> for clearer differentiation from different kinds of pills. [icon] Message from <user> accordingly.

@daniellekirkwood
Copy link
Contributor Author

Edit: ohey now it suddenly fixed itself, it was just real slow previewing that info as I wasn't in the room.

Great. We'll consider this resolved then.

I would also suggest to change the pill from [icon] Message in to Message in [icon] for clearer differentiation from different kinds of pills. [icon] Message from accordingly.

This was the original design but we are not able to implement that due to technical constraints and opted for platform consistency with the designs.

@HarHarLinks
Copy link

Edit: ohey now it suddenly fixed itself, it was just real slow previewing that info as I wasn't in the room.

Great. We'll consider this resolved then.

Actually no, I think it only fixes "itself" when I otherwise make my client aware of the room e.g. manually preview/join/etc it.

@giomfo
Copy link
Member

giomfo commented Apr 4, 2023

Edit: ohey now it suddenly fixed itself, it was just real slow previewing that info as I wasn't in the room.

Great. We'll consider this resolved then.

Actually no, I think it only fixes "itself" when I otherwise make my client aware of the room e.g. manually preview/join/etc it.

@HarHarLinks Hi, this is the expected behaviour. We don't display the room name and avatar if you didn't already retrieve them locally, even if the room is previewable. This is to prevent us from displaying inappropriate content

@HarHarLinks
Copy link

I think I see what you mean, thanks for pointing it out. Personally if i had the choice to enable it regardless, I would.

There's another issue in today's nightly 23040601 clicking in an @room pill results in error 400 as element tries and fails to look up the profile of a userid @room

@giomfo
Copy link
Member

giomfo commented Apr 6, 2023

There's another issue in today's nightly 23040601 clicking in an @room pill results in error 400 as element tries and fails to look up the profile of a userid @room

@HarHarLinks Hi, on which client did you observe this failure? I failed to reproduce on my side

@HarHarLinks
Copy link

https://packages.element.io/debian/pool/main/e/element-nightly/element-nightly_2023040501_amd64.deb which i thought was up to date and didn't realize was outdated by a day so I need to retest with a newer version.

@HarHarLinks
Copy link

HarHarLinks commented Apr 6, 2023

Nope, still an issue on Element web Nightly version: 2023040601
Olm version: 3.2.12

@giomfo
Copy link
Member

giomfo commented Apr 13, 2023

@HarHarLinks sorry we don't reproduce your issue about @room pill.
I will close this issue (this @room pill is out of the initial scope). You may create an issue in Element web repo to provide more details on your issue (a screenshot or a video may help)

@giomfo giomfo closed this as completed Apr 13, 2023
@HarHarLinks
Copy link

i can confirm the issue disappeared somewhere between my reported version and Element Nightly version: 2023041301
Olm version: 3.2.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Matrix.to A-Pills A-Timeline T-Enhancement T-Epic Issue is at Epic level Z-AirFocus Moving issues from GH to AirFocus purposefully using this tag. Z-PS-Request
Projects
None yet
Development

No branches or pull requests

4 participants