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

Implemented FocusCycler#forwardCycle and #backwardCycle events #15050

Merged
merged 7 commits into from
Sep 25, 2023

Conversation

oleq
Copy link
Member

@oleq oleq commented Sep 22, 2023

Suggested merge commit message (convention)

Feature (ui): Implemented FocusCycler#forwardCycle and #backwardCycle events.
Internal (ui): Made SearchResultsView a focus-friendly container. Made SearchInfoView focusable.

@oleq oleq changed the base branch from cf/5436 to master September 22, 2023 13:34
…SearchResultsView a focus-friendly container. Made SearchInfoView focusable.
@oleq oleq force-pushed the cf/5436-nested-focus-cycling branch from 0575ee6 to 3671b8b Compare September 22, 2023 13:36
@oleq oleq changed the base branch from master to cf/5436 September 22, 2023 13:37
Comment on lines 270 to 276
if ( isViewWithFocusableChildren( view ) ) {
if ( childrenFocusDirection === 1 ) {
view.focusFirst();
} else if ( childrenFocusDirection === -1 ) {
view.focusLast();
}
} else {
Copy link
Contributor

@scofalik scofalik Sep 24, 2023

Choose a reason for hiding this comment

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

What if I have a view that can be focused AND has focusable children? Shouldn't we let the view decide what it does upon being focused? EDIT: Actually, this is the definition of ViewWithFocusableChildren :D. So, that view is in theory focusable but it will never actually get the focus because it will always focus its child.

I am not sure if this logic should be inside FocusCycler but this is your domain so you decide.

/**
* Checks whether a view is focusable.
*
* @param view A view to be checked.
*/
function isFocusable( view: View & { focus?: unknown } ) {
return !!( view.focus && isVisible( view.element ) );
function isPlainFocusableView( view: View ): view is FocusableView {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a "plain focusable view"? AFAICS, the implementation did not change. If this is to differentiate "focusable" from "focusable with focusable children", I wouldn't make a change. To me, it felt like maybe "plain focsuable view" is a focusable that is not a "focusable with focusable children". But that's not true. I might not get something, but at the first glance, the change and the new name is confusing.

packages/ckeditor5-ui/src/focuscycler.ts Show resolved Hide resolved
@oleq
Copy link
Member Author

oleq commented Sep 25, 2023

@scofalik I went with your suggestions and moved the decision-making process down to implementers of the FocusableView interface (and the focus() method).

  • focus() has an optional parameter that gives the view extra information about the direction of a traveling focus and it's up to the view to handle this information (or ignore it)
  • I made the events cancellable, thus preventing unnecessary element focus in DOM if a custom logic takes over and focuses something else instead.
  • I reverted the new interface for a view with focusable children. We don't need it (yet).

@oleq
Copy link
Member Author

oleq commented Sep 25, 2023

FYI: Recent changes to this PR (mainly: FocusCycler#focusNext() will no longer re-focus the same view over and over again if there's just one) revealed in unit tests that the focus management in the ColorInputView is totally accidental (but working from the UX standpoint). This is probably due to the fact that the component underwent refactoring this year and nobody realized that.

I addressed this issue and I took the liberty of using the new FC events in table forms to handle focus properly. Changes in 5f9bc87.

@scofalik scofalik merged commit cb9505e into cf/5436 Sep 25, 2023
@scofalik scofalik deleted the cf/5436-nested-focus-cycling branch September 25, 2023 14:22
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.

2 participants