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

Allow Chrome page translator to translate messages in rooms #11113

Conversation

lukaszpolowczyk
Copy link
Contributor

@lukaszpolowczyk lukaszpolowczyk commented Jun 19, 2023

fix element-hq/element-web#25594

The entire application currently has translation disabled using the notranslate class placed in the div#matrixchat element.
The translate class overrides the notranslate class that is in this element's ancestor.

This Pull request adds a translate class to each chat message. Thanks to this, the entire interface is still untouched by the Page Translator (like in Google Chrome), but specific messages, which are quite simple text, are translated.
This increases accessibility for people who do not know a certain language.

Signed-off-by: Łukasz Polowczyk <dev.lukaszpolowczyk@gmail.com>

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

🐛 Bug Fixes

@lukaszpolowczyk lukaszpolowczyk requested a review from a team as a code owner June 19, 2023 17:50
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Jun 19, 2023
@SimonBrandner SimonBrandner requested a review from a team June 20, 2023 14:07
@SimonBrandner
Copy link
Contributor

This is going to need a product review

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Apologies for the long delay in reviewing this.

This is going to need a product review

First step of a review here is for someone to explain what this PR actually does, functionally speaking. @lukaszpolowczyk, please could you update the description of the PR to explain how this changes the user experience of the application. I see it links to an issue but there is a great deal of discussion in that issue and a concise summary would be useful

Also, please note that we require sign-off on all contributions. Please take a look at https://github.com/element-hq/element-web/blob/develop/CONTRIBUTING.md#sign-off and update the PR description with a "Signed-off-by" line.

@richvdh richvdh removed request for SimonBrandner and a team January 10, 2024 18:13
@richvdh
Copy link
Member

richvdh commented Jan 10, 2024

Also, @SimonBrandner, if you have time: it would be good to record why you believe it needs product review?

@richvdh richvdh added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Jan 10, 2024
@lukaszpolowczyk
Copy link
Contributor Author

@SimonBrandner @SimonBrandner What else should I do? I want this PR to be pushed through.

@SimonBrandner
Copy link
Contributor

Sorry, I missed this, I am no longer working at Element... @richvdh, it probably ought to be checked if this is an expected behaviour for a chat app (I am not sure about that...), that is why I am suggesting a product review

@lukaszpolowczyk
Copy link
Contributor Author

it probably ought to be checked if this is an expected behaviour for a chat app (I am not sure about that...)

@SimonBrandner Eh, I don't understand this approach to the matter.
Just don't make people's lives harder by artificially blocking a helpful tool.
If someone with a disability wanted to use chat, would you artificially block him from using a tool that helps him chat? What kind of approach to the matter is this, I can't understand it? Isn't not knowing a certain language a certain "disability"? Theoretically it is possible to learn a language, but in practice some people are not able to. You can translate the entire Internet with this translation tool, but not this particular chat - what's the point? This is putting someone under the microscope and making life difficult. This can be bypassed, but in a very inconvenient way - that's all you achieve. Why are you doing this?
I know that this is only your opinion, someone else may have a different opinion and will accept my amendment, but I am turning to you as an example because I want to understand what really drives you?

@SimonBrandner
Copy link
Contributor

I feel a little strawmanned - I do not wish to make anyone's life harder. I am not against this feature.

What I am saying is: I do not know if this is expected behaviour for Element and such a thing was not up to me to decide when I was reviewing the PR, that is why I called for the product review. If @richvdh merges this, I am absolutely fine with it!

@@ -399,6 +399,7 @@ export function bodyToHtml(content: IContent, highlights: Optional<string[]>, op
"mx_EventTile_body": true,
"mx_EventTile_bigEmoji": emojiBody,
"markdown-body": isHtmlMessage && !emojiBody,
"translate": true,
Copy link
Member

Choose a reason for hiding this comment

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

Please could you add a comment explaining a bit more about what this does, and why it is necessary? Surely it isn't normally necessary to mark things for translation explicitly?

Do I understand correctly that this is overriding a notranslate class somewhere else? Please say that in a comment if so.

Basically: imagine you are a developer reading this code for the first time, without the benefit of having read all of the conversation on element-hq/element-web#25594, and that you are trying to understand what this is for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I understand correctly that this is overriding a notranslate class somewhere else? Please say that in a comment if so.

@richvdh Yes.

The entire application currently has translation disabled using the notranslate class placed in the div#matrixchat element.
The translate class overrides the notranslate class that is in this element's ancestor.

@richvdh
Copy link
Member

richvdh commented May 9, 2024

@SimonBrandner: thanks very much for the update. @lukaszpolowczyk: please be patient, nobody is trying to make life difficult unnecessarily.

Anyway. @lukaszpolowczyk:

Thanks for updating the PR summary as requested; that is helpful.

So far, it is not possible to use the translator on the element.io website.

The element.io website is https://element.io/. I think you mean "the Element-Web application" (which is hosted at https://app.element.io, among many other places)?

Remaining changes needed here:

  1. Please add a comment to the code, as requested above.
  2. Please do read https://github.com/element-hq/element-web/blob/develop/CONTRIBUTING.md#sign-off. Unfortunately we are unable to accept even trivial changes without sign-off.
  3. It looks like the jest tests are failing due to changes in the snapshots. Obviously, we cannot merge changes that will break our CI. Please update the tests so that they continue to pass. If you're having trouble, please drop by the #element-dev:matrix.org room to ask for help.

Once you've made the necessary changes, do feel free to write a message here to remind us to look at them again.

@lukaszpolowczyk
Copy link
Contributor Author

@richvdh I added Signed-off to commit.
But I will also add it to Pull Request.
I added a more detailed description. I hope this will be it?

Should I add a more detailed commit somewhere else? Because I don't understand this: #11113 (comment)

@richvdh richvdh self-requested a review May 13, 2024 10:25
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

@richvdh I added Signed-off to commit. But I will also add it to Pull Request.

Thank you. [Your commit does not include a Signed-off line. But that's fine, it is sufficient to put it in the PR].

Should I add a more detailed commit somewhere else? Because I don't understand this: #11113 (comment)

Please add a comment to the code.

@richvdh richvdh changed the title support message translation in chat Allow Chrome page tranlator to translate individual messages Jul 11, 2024
@richvdh richvdh changed the title Allow Chrome page tranlator to translate individual messages Allow Chrome page translator to translate individual messages Jul 11, 2024
@richvdh richvdh changed the title Allow Chrome page translator to translate individual messages Allow Chrome page translator to translate messages in rooms Jul 11, 2024
@richvdh richvdh added this pull request to the merge queue Jul 25, 2024
Merged via the queue into matrix-org:develop with commit c1420ba Jul 25, 2024
26 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No support for Page Translation in Google Chrome
4 participants