Update preventScroll option in focus and focusElement

[nakjun12]

Enact-DCO-1.0-Signed-off-by: Nakjun Hwang [email protected]

Checklist

• I have read and understand the contribution guide
• A CHANGELOG entry is included
• At least one test case is included for this feature or bug fix
• Documentation was added or is not needed
• This is an API breaking change

Issue Resolved / Feature Added

• Improved scrolling behavior by adding a preventScroll parameter to the focusElement and extending focus in spotlight/Spotlight to include containerOption with preventScroll.

Resolution

• The code works as intended, enabling more precise control over the scrolling behavior when focusing elements. This change was implemented to enhance the user navigation experience.
• The preventScroll parameter was introduced because, when using spotlight.focus, the forced scrolling behavior caused discomfort in the user experience. Consequently, this change was implemented to improve the user experience by granting greater control over preventScroll in enact.

Additional Considerations

Separate PRs for focusElement and spotlight.focus
Considering splitting this PR into two: one for focusElement and another for spotlight.focus. This would allow for more focused review and testing of each feature.

Links

Focus with and without scrolling

Comments

To avoid affecting existing code:

A 4th argument was introduced in focusElement specifically for the preventScroll parameter.
For spotlight.focus, preventScroll was added to the existing containerOption.

Extensibility
The preventScroll parameter is designed for integration into other methods using focusElement.

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (b285309) 82.21% compared to head (716220c) 82.21%.

❗ Current head 716220c differs from pull request most recent head b17f48b. Consider uploading reports for the commit b17f48b to get more accurate results

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3177   +/-   ##
========================================
  Coverage    82.21%   82.21%           
========================================
  Files          155      155           
  Lines         7141     7142    +1     
  Branches      1885     1887    +2     
========================================
+ Hits          5871     5872    +1     
  Misses         997      997           
  Partials       273      273           

Files Changed	Coverage Δ
packages/spotlight/src/spotlight.js	43.73% <100.00%> (+0.17%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[seunghoh]

@nakjun12 Thanks for the PR. Do you have any user scenarios you mentioned about "the forced scrolling behavior caused discomfort in the user experience"? It would be appreciated if you also explain the scenario how we can use this preventScroll with Spotlight focus?

[nakjun12]

@seunghoh Thank you for your review.

I encountered the issue while implementing an app with a user interface similar to Netflix.

During development, I noticed that scroll events for navigating up and down were conflicting with the automatic scrolling triggered by focus events. This conflict led to behaviors that diverged from the intended user experience.

To address this, I introduced the preventScroll option to give developers more control over the focus-scrolling behavior. This would allow developers to decide whether they want to handle scrolling through custom scroll events or allow the automatic focus-based scrolling.

If you have any more questions or need further clarification, feel free to ask. Thank you for considering my PR.

[0x64]

I noticed that scroll events for navigating up and down were conflicting with the automatic scrolling triggered by focus events.

@nakjun12
Thank you for PR!

Could you please let us know what is the detail background scenario? We're working with a lot of media app developers and normally the issue is not a scrolling itself but scrolling distance so devs (at least we know) resolve their issue with Spotlight container. I'm not sure that your issue is the same as those, so it would be very helpful to understand what is your app's situation if you let us know.

We may accept your idea if the scenario is general and the issue is inevitable, and we're asking you about scenario to determine whether the new API would mislead app developers to write a kind of undesired pattern. I believe that you understand framework maintainers take care of app patterns seriously.

[nakjun12]

@0x64 Thank you for reading my comments, and I understand that you're managing Spotlight carefully.

While developing an app with a UI similar to Netflix, I discovered conflicts between scrollIntoView and spotlight.focus. Specifically, I encountered issues when using spotlight.focus to navigate to areas outside of the currently visible screen area. The movement was unnatural, and I also had issue where animations that were supposed to occur during the transition would complete prematurely.

I addressed these issues with the preventScroll option, but this resulted in a more complex codebase as I had to control focus directly without spotlight.focus.

I aim to allow for more precise control over focus and scrolling behavior, even directly within spotlight.focus.

One concern I have is that my PR addresses both spotlight.focus and focusElement. If during the review process it's determined that addressing both in a single PR complicates the review, I'm open to considering splitting it into separate PRs.

Thank you once again for considering my PR. If you have any further questions or need clarification, please don't hesitate to ask.

[0x64]

@nakjun12 Sorry for late response. What is scroll.interview, by the way? (It looks a typo of scrollIntoView by auto-completion or something.)

[seunghoh]

@nakjun12 Thanks for your detailed explanation. It sounds makes sense. I can't tell your PR will be merged immediately due to our version maintenance schedule, but we will definitely merge your PR when we start the next version(It contains new APIs, refactoring, API changes etc.)

[nakjun12]

@0x64 @seunghoh
I've corrected the typo. Thank you for bringing it to my attention.
I appreciate both of you. Have a great day! 😄

[0x64]

@nakjun12 If you don't mind, could we update your branch when we start the next version? We just reviewed briefly, and we may need to change API after in-depth review regarding API consistency. We don't want to make another branch for your suggestion, so we will update your branch directly in this pull request if you agree.

[nakjun12]

@0x64 Of course, feel free to update my branch.

[0x64]

Closing as a refined PR #3277 is ready. The original commit stays in the new PR.