[web] Fix closing MultimediaModal on ESC press bug
ClosedPublic
Actions

Authored by • patryk on Jul 14 2023, 2:28 AM.

Details

Reviewers

bartek
tomek
ashoat

Commits

rCOMM8ba42dce166a: [web] Fix closing MultimediaModal on ESC press bug

Summary

Fix for ENG-4327.

This diff solves the issue, where closing MultimediaModal with the ESC key was impossible.

Test Plan

In chat, click on the image and when MultimediaModal is shown, check if ESC key press closes it. Tested on unencrypted and encrypted images.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• patryk created this revision.Jul 14 2023, 2:28 AM

• patryk held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 14 2023, 2:28 AM

Move outline:none; to separate selector.

• patryk added inline comments.Jul 14 2023, 2:42 AM

web/media/media.css
106–108 ↗	(On Diff #28686)	When we focus on element, blue border is created by default. We want to hide it for `.mediaContainer`. Before change (tested browser: Safari): After change:
web/media/multimedia-modal.react.js
133–134 ↗	(On Diff #28686)	I moved these attributes from parent div, because we run `.focus()` on this child div here. That was the reason why ESC key didn't work.

• patryk attached a referenced file: F635448: after.png. (Show Details)Jul 14 2023, 2:42 AM

• patryk attached a referenced file: F635447: reason1.png. (Show Details)

Harbormaster completed remote builds in B20931: Diff 28685.Jul 14 2023, 2:46 AM

Harbormaster completed remote builds in B20932: Diff 28686.Jul 14 2023, 2:51 AM

• patryk edited the summary of this revision. (Show Details)Jul 14 2023, 2:53 AM

• patryk edited the test plan for this revision. (Show Details)

• patryk added reviewers: bartek, tomek.

• patryk added a reviewer: ashoat.

• patryk edited the test plan for this revision. (Show Details)Jul 14 2023, 3:27 AM

• patryk added inline comments.Jul 14 2023, 3:30 AM

web/media/media.css
106–108 ↗	(On Diff #28686)	Note: also tested on Firefox and Chrome, and it is working as expected. Also, this selector doesn't affect other components.

• patryk published this revision for review.Jul 14 2023, 3:31 AM

Thanks for this diff! In the future we should probably move away from you putting me as a reviewer on all of your diffs, but I'm happy to help review this one :)

web/media/media.css
106–108 ↗	(On Diff #28686)	This seems like a good change, but I don't understand what it has to do with the ESC key. Should this have been a separate diff?

This revision is now accepted and ready to land.Jul 14 2023, 7:56 AM

• patryk added inline comments.Jul 17 2023, 5:51 AM

web/media/media.css
106–108 ↗	(On Diff #28686)	I just weirdly concluded that because we detect ESC key press by focusing on div, it's somehow connected to this diff, but you're right, it should have been a separate diff.