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

The dropdown menu component should not cause an editor blur if used in a BalloonToolbar #17140

Closed
wants to merge 14 commits into from

Conversation

oleq
Copy link
Member

@oleq oleq commented Sep 23, 2024

Suggested merge commit message (convention)

Fix (ui): The dropdown menu component should not cause an editor blur if used in a BalloonToolbar while the user hovers a nested menu.

Feature (utils): Made FocusTracker extendable with other FocusTracker instances to allow for logical focus tracking across separate DOM sub-trees.


Additional information

Closes https://github.com/cksource/ckeditor5-commercial/issues/6633.

* * If a {@link module:ui/view~View} instance is passed that has no `FocusTracker` (**not** a {@link ~ViewWithFocusTracker}),
* its {@link module:ui/view~View#element} is used to track focus like any other DOM element.
*/
public add( elementOrView: Element | View ): void {
Copy link
Member Author

Choose a reason for hiding this comment

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

This add( elementOrView: Element | View ) is convenient for integrators because they don't have to think much but it's quite hard to document and explain in the API because it's the view's FT instance we're after, not the view itself. It does not tell when full story.

A more explicit add( otherFocusTracker: FocusTracker ) would be more in line with what we want to do. It would also make more sense with #externalFocusTrackers. But it would mean that the chore of checking if ( isViewWithFocusTracker( someView ) ) { someFocusTracker.add( someView.focusTracker ) } would be on the integrator's side (and repeat a lot), a bad DX.

return 'focusTracker' in view && view.focusTracker instanceof FocusTracker;
}

function isElement( value: any ): value is Element {
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to pollute this PR with unrelated changes, but it seems we have several similar functions scattered in the codebase. I'll create a follow-up and move this to utils/src/dom to share across the project.

* Starts tracking the specified element.
* List of external focus trackers that contribute to the state of this focus tracker. See {@link #add} to learn more.
*/
public get externalFocusTrackers(): Array<FocusTracker> {
Copy link
Member Author

@oleq oleq Sep 23, 2024

Choose a reason for hiding this comment

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

I decided I didn't want to extend #elements with elements of external focus trackers to keep the previous API contract (avoid BCs). I came up with this property that will allow integrators to dig deeper if they want, though.

Copy link
Member Author

@oleq oleq Sep 23, 2024

Choose a reason for hiding this comment

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

Until now #focusedElement always was one of #elements.

But now #focusedElement may point to a DOM element outside of #elements if an external FT's element is focused. I didn't want to leave #focusedElement === null if #isFocused === true in that situation so the integrators may need to access #externalFocusTrackers to learn more.

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.

1 participant