Skip to content

Dialog - useHiddenLocation, added reduce motion listener for testing on Android. #3739

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Dialog - useHiddenLocation, added reduce motion listener for testing on Android. #3739

wants to merge 9 commits into from

Conversation

adids1221
Copy link
Contributor

@adids1221 adids1221 commented May 25, 2025 •
edited
Loading

Description

Dialog component only showed overlay (no content) when isReduceMotionEnabled was set to true on Android, content was positioned off-screen with no animation to bring it into view.

Fix:
Centralized accessibility state: Added isReduceMotionEnabled to Constants.accessibility following the existing pattern.
Fixed Dialog animations: Modified Dialog's open() and close() functions to respect reduce motion preference

Changes:

  • Constants - Added isReduceMotionEnabled property and initialization
  • Dialog - Added reduce motion checks in animation functions
  • JestSetup - Fixed AccessibilityInfo mocking
  • Fixed multiple test files affected by AccessibilityInfo mocking issues

React native docs

Changelog

Fix Dialog component only showed overlay (no content) when isReduceMotionEnabled was set to true on Android.

Additional info

Slack thread in support channel

@adids1221 adids1221 requested a review from M-i-k-e-l May 25, 2025 14:11
M-i-k-e-l
M-i-k-e-l reviewed May 27, 2025
View reviewed changes
src/incubator/hooks/useHiddenLocation.ts Outdated
const [reduceMotionEnabled, setReduceMotionEnabled] = useState(false);

useEffect(() => {
AccessibilityInfo.isReduceMotionEnabled().then(setReduceMotionEnabled);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Look in Constants -> accessibility is set with a promise, we can probably add the info there IMO.

src/incubator/hooks/useHiddenLocation.ts Outdated
const getHiddenLocation = ({
x = 0,
y = 0,
width = Constants.screenWidth,
height = Constants.windowHeight,
wasMeasured = wasMeasuredDefaultValue
}): HiddenLocation => {
if (Constants.isAndroid && reduceMotionEnabled) {
return defaultHiddenLocation;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this just getHiddenLocation({})?
Maybe you can do it some other way?

@M-i-k-e-l M-i-k-e-l assigned adids1221 and unassigned M-i-k-e-l May 27, 2025
@adids1221 adids1221 requested a review from M-i-k-e-l June 4, 2025 09:51
@adids1221 adids1221 assigned M-i-k-e-l and unassigned adids1221 Jun 4, 2025
@adids1221
Copy link
Contributor Author

adids1221 commented Jun 4, 2025 •
edited
Loading

@M-i-k-e-l After changing the useHiddenLocation the issue still reproduce.
The fix is in Dialog open, close functions, the visibility is now not Animated when isReduceMotionEnabled set to true.

@M-i-k-e-l
Copy link
Collaborator

@M-i-k-e-l After changing the useHiddenLocation the issue still reproduce. The fix is in Dialog open, close functions, the visibility is now not Animated when isReduceMotionEnabled set to true.

@adids1221

  1. WDYM still reprodices? We've tested it, didn't we?
  2. The changes to include the test seem to be too big. Furthermore, if you'll add the test to master (without the jest.spyOn(AccessibilityInfo, 'isReduceMotionEnabled').mockResolvedValue(true);) it'll still pass, so I don't think it actually tests anything. If you really want to test it you can probably add a Detox test in the private repo.

@M-i-k-e-l M-i-k-e-l assigned adids1221 and unassigned M-i-k-e-l Jun 5, 2025
@adids1221
Copy link
Contributor Author

adids1221 commented Jun 8, 2025 •
edited
Loading

@M-i-k-e-l After changing the useHiddenLocation the issue still reproduce. The fix is in Dialog open, close functions, the visibility is now not Animated when isReduceMotionEnabled set to true.

@adids1221

  1. WDYM still reprodices? We've tested it, didn't we?
  2. The changes to include the test seem to be too big. Furthermore, if you'll add the test to master (without the jest.spyOn(AccessibilityInfo, 'isReduceMotionEnabled').mockResolvedValue(true);) it'll still pass, so I don't think it actually tests anything. If you really want to test it you can probably add a Detox test in the private repo.
  1. Yes Iv'e tested it, but when I checked it again I saw that it reproduced after couple of times.
    The solution should be in the Animation part since this is the issue.
  2. I'll open a ticket for the Detox test (MADS-4749).

@adids1221 adids1221 assigned M-i-k-e-l and unassigned adids1221 Jun 8, 2025
@M-i-k-e-l M-i-k-e-l assigned adids1221 and unassigned M-i-k-e-l Jun 8, 2025
@M-i-k-e-l
Copy link
Collaborator

Let's verify it is reproduced with the old changes, if it does let's only keep the fix here and test in private

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

@M-i-k-e-l M-i-k-e-l Awaiting requested review from M-i-k-e-l

At least 1 approving review is required to merge this pull request.

Assignees

@adids1221 adids1221

Labels
waiting-for-response
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@adids1221 @M-i-k-e-l