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

Fix move/close menu not showing on draggable video tile when ctrl+alt+space shortcut is used #5652

Merged
merged 3 commits into from
Feb 19, 2025

Conversation

JamesBurnside
Copy link
Member

@JamesBurnside JamesBurnside commented Feb 15, 2025

What

  • Remove use of useWindow, in react strict mode it is coming back null
  • Replace use of internalEvents.on with add/removeEventListener, internalEvents.on simply wasn't working in react 18 strict mode
  • Change toggle open menu to just set open menu as strict mode was executing twice so caused it to open and instantly close
  • Add a forceUpdateCallback that triggers a rerender to remove the move icon, the icon wasn't being removed in react18 due to the render optimizations; there is no set state triggering a re-render and no longer an unrelated re-render occurring
  • Add a keyEventElement so window doesn't have to be used (not being used by component or composite currently as it would be a larger change with an API change, no regression in continuing to use window object for now)

Why

React 18 strict mode broke the a11y menu for the draggable tile, see recording below for menu being fixed

How Tested

move-keyboard-recording.mp4

Copy link
Contributor

Copy link
Contributor

github-actions bot commented Feb 15, 2025

@azure/communication-react jest test coverage for stable.

Lines Statements Functions Branches
Base 28048 / 44663
62.79%
28048 / 44663
62.79%
786 / 1436
54.73%
2355 / 3725
63.22%
Current 28056 / 44674
62.8%
28056 / 44674
62.8%
786 / 1436
54.73%
2355 / 3725
63.22%
Diff 8 / 11
0.01%
8 / 11
0.01%
0 / 0
0%
0 / 0
0%

Copy link
Contributor

github-actions bot commented Feb 15, 2025

@azure/communication-react jest test coverage for beta.

Lines Statements Functions Branches
Base 58282 / 94195
61.87%
58282 / 94195
61.87%
1177 / 2693
43.7%
3540 / 5832
60.69%
Current 58253 / 94205
61.83%
58253 / 94205
61.83%
1177 / 2693
43.7%
3561 / 5842
60.95%
Diff -29 / 10
-0.04%
-29 / 10
-0.04%
0 / 0
0%
21 / 10
0.26%

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 3 changed files in this pull request and generated no comments.

Files not reviewed (2)
  • change-beta/@azure-communication-react-efaa1c36-cd40-48ac-8905-e7c88a135523.json: Language not supported
  • change/@azure-communication-react-efaa1c36-cd40-48ac-8905-e7c88a135523.json: Language not supported
Comments suppressed due to low confidence (2)

packages/react-components/src/components/ModalClone/ModalClone.tsx:93

  • Changing the event type from React.KeyboardEvent to KeyboardEvent might cause type mismatches or runtime errors if the function is called with a React.KeyboardEvent. Ensure all call sites are updated accordingly.
const getMoveDelta = (ev: KeyboardEvent): number => {

packages/react-components/src/components/ModalClone/ModalClone.tsx:154

  • Using window as the default value for keyEventElement might lead to unexpected behavior in environments where window is not available, such as server-side rendering. Ensure window is always available or provide a fallback mechanism.
keyEventElement = window
@JamesBurnside JamesBurnside enabled auto-merge (squash) February 19, 2025 16:32
Copy link
Contributor

CallWithChat bundle size is not changed.

  • Current size: 12401112
  • Base size: 12401112
  • Diff size: 0

Copy link
Contributor

Chat bundle size is not changed.

  • Current size: 1777281
  • Base size: 1777281
  • Diff size: 0

Copy link
Contributor

Calling bundle size is not changed.

  • Current size: 12401100
  • Base size: 12401100
  • Diff size: 0

Copy link
Contributor

@JamesBurnside JamesBurnside merged commit 6f3fd7a into main Feb 19, 2025
41 checks passed
@JamesBurnside JamesBurnside deleted the jaburnsi/fix-modal-accessibility-menu branch February 19, 2025 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants