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 playback history window doesn't respect reduce motion, #4870 #4871

Merged
merged 5 commits into from
May 28, 2024

Conversation

low-batt
Copy link
Contributor

@low-batt low-batt commented Apr 7, 2024

This commit will change IINA to adhere to the Apple Human Interface Guidelines with respects to the macOS Reduced motion setting. With these changes IINA will reduce, not eliminate, animations when the macOS Reduced motion setting is enabled. When this setting is enabled IINA will fade in the sidebar instead of sliding it into view. All other changes to animation behavior, such as how IINA transitions in and out of full screen mode, are provide by AppKit.

For users who are hyper sensitive to motion this commit adds a Disable animations setting to the UI settings tab in a new Accessibility section at the bottom of the panel. Enabling this setting will eliminate many, but not all, animated transition effects.


Description:

This commit will:
- Add a new class OutlineView that extends NSOutlineView adding support
  for reducing motion
- Change HistoryWindowController.xib to use OutlineView instead of
  NSOutlineView

This suppresses the sliding animation when collapsing and expanding
rows in the playback history window.
@low-batt low-batt linked an issue Apr 7, 2024 that may be closed by this pull request
1 task
@low-batt low-batt requested a review from uiryuu April 7, 2024 04:22
Copy link
Member

@saagarjha saagarjha left a comment

Choose a reason for hiding this comment

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

Reduce motion doesn't mean "turn off all animations", it means "avoid problematic transitions that are likely to cause people to feel disoriented". This animation provides important context to a user interaction and seems like a poor choice to disable unless it is causing issues for people.

@low-batt
Copy link
Contributor Author

low-batt commented Apr 7, 2024

Apple has recognized that sliding is causing issues for such people. The Human Interface Guidelines in the Motion section provides this guidance:

  • Replace a slide with a fade to avoid motion

I couldn't find an easy way to make NSOutlineView first fade the item before making it just disappear. Is there a way to do that?

I could break the connection with the macOS reduce motion setting and create a Use animations checkbox in IINA's general settings in the behavior section. Should I implement that?

@saagarjha
Copy link
Member

IMO if you think this would be a problem, we should file a feedback and get Apple to implement it in NSOutlineView rather than trying to roll it ourselves.

@low-batt
Copy link
Contributor Author

low-batt commented Apr 8, 2024

I am going to move this to a draft until I have time to add an IINA setting for controlling animations.

It used to be that people sensitive to motion were using the defaults command to set macOS properties that controlled AppKit animations. I think it was starting with OS X Mountain Lion that some of these properties stopped working. More properties stopped working with the releases that followed. Since that time people have been giving Apple feedback on this. I'm expecting the feedback goes to the group that handles accessibility. Relative to the other kinds of impairments this group must address I would expect reducing motion sickness to be a low priority.

But recently I have noticed changes. My guess is that two things have brought some attention to this problem. The first was the need to support the CSS prefers-reduced-motion feature in Safari and Apple's web pages. The second is that with visionOS Apple has to worry about animations contributing to virtual reality sickness. Possibility this is why the accessibility portion of the HIG that covers motion was updated and enlarged. There is also now a separate motion section. In that section under Best Practices it says:

Make motion optional. There are several reasons why people might not experience movement in your app, so it’s essential to avoid using it as the only way to communicate important information. For example, people can turn on the Reduce Motion accessibility setting to minimize or eliminate animations. For guidance, see Motion.

I don't remember Apple ever mentioning eliminate animations in the HIG before. I'm hoping Apple is starting to recognize the problems sliding and zooming trigger in some people.

It definitely would be good to get Apple to enhance NSOutlineView, but that won't provide a solution for older versions of macOS.

@low-batt low-batt marked this pull request as draft April 8, 2024 22:46
This commit will change IINA to adhere to the Apple Human Interface
Guidelines with respects to the macOS "Reduce motion" setting. With
these changes IINA will reduce, not eliminate, animations when the macOS
reduce motion setting is enabled. When this setting is enabled IINA will
fade in the sidebar instead of sliding it into view. All other changes
to animation behavior, such as how IINA transitions in and out of full
screen mode, are provide by AppKit.

For users who are hyper sensitive to motion this commit adds an
"Enable animations" setting to the UI settings tab in a new "Animations"
section at the bottom of the panel. Disabling this setting will
eliminate many, but not all, animated transition effects.
@low-batt
Copy link
Contributor Author

With Reduced motion now only causing IINA to replace the slide animation with a fade when showing/hiding the sidebar, I'm sure Apple would approve of IINA's behavior when this setting is enabled.

Reviewers should take a close look at the showSideBar and hideSideBar methods in MainWindowController. I had a hard time getting the animations to switch between sliding and fading. Is there a cleaner way to code this?

@low-batt
Copy link
Contributor Author

This is what the new setting looks like:
animation-setting

@low-batt low-batt marked this pull request as ready for review May 13, 2024 22:18
This commit will change IINA to adhere to the Apple Human Interface
Guidelines with respects to the macOS "Reduce motion" setting. With
these changes IINA will reduce, not eliminate, animations when the macOS
reduce motion setting is enabled. When this setting is enabled IINA will
fade in the sidebar instead of sliding it into view. All other changes
to animation behavior, such as how IINA transitions in and out of full
screen mode, are provide by AppKit.

For users who are hyper sensitive to motion this commit adds an
"Enable animations" setting to the UI settings tab in a new "Animations"
section at the bottom of the panel. Disabling this setting will
eliminate many, but not all, animated transition effects.
@low-batt
Copy link
Contributor Author

After thinking about the code some more I made one more fix.

@svobs
Copy link
Contributor

svobs commented May 14, 2024

Reviewers should take a close look at the showSideBar and hideSideBar methods in MainWindowController. I had a hard time getting the animations to switch between sliding and fading. Is there a cleaner way to code this?

Trying out the built product, the fade effect seems to be working as desired... But I see you're only manipulating sideBarView.alphaValue to do an explicit fade in hideSideBar and not showSideBar. Is there a reason you didn't use alphaValue for both? Looks like the fade is still happening for the latter case via an implicit animation which results from sideBarView.isHidden being toggled inside an animation group. I've found this mechanism to be less robust in general than when using alphaValue, though in this case it's probably safe enough because the layout changes being made are relatively simple.

@low-batt
Copy link
Contributor Author

low-batt commented May 14, 2024

I originally attempted to just use alphaValue. I had showSideBar set alphaValue to zero and then animate it to 1 to fade-in the sidebar. That did not work. The sidebar was fully opaque from the start. To get that to work I had to set alphaValue to zero outside of showSideBar. That complicated the code a lot. So I switched to animating isHidden to false to fade-in the sidebar. That worked. But animating isHidden to true did not fade-out the sidebar. It just immediately disappeared. I found others discussing this. They had to use alphaValue to get the fade-out to work. I tried that and it worked. It makes no sense to me why isHidden works for fade-in but not for fade-out. I was hoping somebody would know a trick to get this to work in a cleaner way.

@uiryuu
Copy link
Member

uiryuu commented May 25, 2024

I really don't think this is necessary. For one, I support @saagarjha's opinion. We really shouldn't mess NSOutlineView up by ourselves. With reduce motion enabled in the system settings, I don't observe one app (including Apple's apps, like Music, Xcode) removes the motion in the NSOutlineView. Besides, introducing another "enable animations" in the IINA settings, which only a very small number of users will use, along side the "reduce motion" already exists in the system accessibility setting, will cause further confusion to users.

This commit will change IINA to adhere to the Apple Human Interface
Guidelines with respects to the macOS "Reduce motion" setting. With
these changes IINA will reduce, not eliminate, animations when the macOS
reduce motion setting is enabled. When this setting is enabled IINA will
fade in the sidebar instead of sliding it into view. All other changes
to animation behavior, such as how IINA transitions in and out of full
screen mode, are provide by AppKit.

For users who are hyper sensitive to motion this commit adds an
"Disable animations" setting to the UI settings tab in a new
"Accessibility" section at the bottom of the panel. Enabling this
setting will eliminate many, but not all, animated transition effects.
This commit will change IINA to adhere to the Apple Human Interface
Guidelines with respects to the macOS "Reduce motion" setting. With
these changes IINA will reduce, not eliminate, animations when the macOS
reduce motion setting is enabled. When this setting is enabled IINA will
fade in the sidebar instead of sliding it into view. All other changes
to animation behavior, such as how IINA transitions in and out of full
screen mode, are provide by AppKit.

For users who are hyper sensitive to motion this commit adds an
"Disable animations" setting to the UI settings tab in a new
"Accessibility" section at the bottom of the panel. Enabling this
setting will eliminate many, but not all, animated transition effects.
@low-batt
Copy link
Contributor Author

Offline @uiryuu gave me some suggestions on how to make clear what this setting does to avoid confusing users.

The latest commit changes the section at the end of the UI tab to be Accessibility. The setting toggle is now Disable animations. There is a help button that links to the HIG Motion section. The toggle has the following text under it:

When the macOS Reduce Motion accessibility setting is enabled IINA will reduce user interface animations that are known to cause problems for those with vestibular disorders. Should that be insufficient this setting can be used to reduce motion further by eliminating user interface animations.

It now looks like this:
accessibility

@uiryuu uiryuu merged commit bcb5c2b into develop May 28, 2024
1 check passed
@uiryuu uiryuu deleted the history-animations branch May 28, 2024 11:53
@svobs svobs mentioned this pull request May 29, 2024
2 tasks
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.

Playback history window doesn't respect reduce motion
4 participants