Skip to content

Conversation

@ioalexander
Copy link
Contributor

@ioalexander ioalexander commented Jul 10, 2025

Enhancement: Prevent browser default behavior when pressing Ctrl + S on an open image, which would otherwise trigger image saving.

image

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • [X ] Linter and other CI checks pass.
  • I have licensed the changes to Element by completing the Contributor License Agreement (CLA)

@ioalexander ioalexander requested a review from a team as a code owner July 10, 2025 20:28
@ioalexander ioalexander requested review from dbkr and t3chguy July 10, 2025 20:28
@CLAassistant
Copy link

CLAassistant commented Jul 10, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Few comments, but also this would need a test of some kind. Nice feature though.

@dbkr
Copy link
Member

dbkr commented Jul 14, 2025

Actually, having done that round of review, I've just noticed this really should be using the same code as the existing download image button on the message action bar: https://github.com/element-hq/element-web/blob/develop/src/components/views/messages/DownloadActionButton.tsx - Ithink the right way to do this would be to factor that code out and make it re-usable.

@ioalexander
Copy link
Contributor Author

Thanks for flagging this @dbkr

I’ve refactored the download logic into useDownloadMedia hook.

I’ve added a new test to cover the Ctrl+S keyboard shortcut in the ImageView. CTRL+S now triggers the download in the DownloadButton

Looking forward for a review!

@ioalexander
Copy link
Contributor Author

@dbkr I've merged in the updates and removed the forwardRef usage.
I’m seeing an i18n failure now - I'm not sure if it’s related to my changes. Is this fine as it should be?

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Ah, you read my mind: I was staring at the function passed back from forwardRef for quite a while trying to work out what it was doing. Thanks, I think this is much easier to understand.

Also thanks for spotting and merging in the upstream fix (which I noticed thanks to reviewing this PR!)

For the i18n check, you should just be able to run yarn i18n and it will scan all the files for translatable strings and put them in the file for you.

@ioalexander
Copy link
Contributor Author

@dbkr i18n applied

@dbkr
Copy link
Member

dbkr commented Jul 16, 2025

Thanks! I think our sonarcloud is unhappy because you're trying to contribute from your develop branch (I though we had a thing that warns you but it doesn't seem to have triggered). Would you mind changing to a different branch on your fork, then I think this is good to go.

@ioalexander
Copy link
Contributor Author

@dbkr here you go: #30330
Closing this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-Enhancement 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.

3 participants