Skip to content

Conversation

ChronosSF
Copy link
Member

@ChronosSF ChronosSF commented Oct 8, 2024

Closes #14842 #14733

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

@ChronosSF ChronosSF added ❌ status: awaiting-test PRs awaiting manual verification version: 18.2.x labels Oct 8, 2024
@dkamburov dkamburov assigned dkamburov and IMinchev64 and unassigned dkamburov Oct 10, 2024
@IMinchev64 IMinchev64 added 💥 status: in-test PRs currently being tested ✅ status: verified Applies to PRs that have passed manual verification and removed ❌ status: awaiting-test PRs awaiting manual verification 💥 status: in-test PRs currently being tested labels Oct 15, 2024
// simulate the process of focusing out of the cell so that the active element is cleared
// while this should naturally happen when clicking a button (as a most likely endEdit called)
// the intentional focusout ignore during edit mode prevents the expected behavior
this.onFocusOut({ target: this.tbody.nativeElement } as unknown as FocusEvent);
Copy link
Member

@damyanpetev damyanpetev Oct 16, 2024

Choose a reason for hiding this comment

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

Eh, I think it might be a bit too risky to assume all calls to this API are coming when the Grid is focused out and clear selection.
Actually, there's a case for the opposite that currently has an issue. See Row Edit sample in dev demos, check Custom Template (it should have been using endEdit() as exposed by the template context, but I digress). Point being the buttons on the row edit overlay are intended to call into endEdit but loose focus breaking KB nav. Compare w/ what the default template use - the internal endRowEditTabStop that checks for active node and returns focus to the grid tbody. Note that check is also silly and fails if the last active node is header, etc.

My thinking is for this method to handle both cases correctly, it should probably check the focused state of the grid before restoring focus or clearing active cell. Something in the lines of:

        const activeNode = this.navigation.activeNode;
        const document = this.nativeElement?.getRootNode() as Document | ShadowRoot;

        if (!activeNode) {
            return success;
        }

        if (this.nativeElement?.contains(document.activeElement) && activeNode.row !== -1) {
            // restore focus for navigation, TODO: navigation method to handle all cases...
            this.tbody.nativeElement.focus();
        } else {
            // grid already lost focus, clear active
            this.navigation.lastActiveNode = activeNode;
            this.navigation.activeNode = {} as IActiveNode;
            this.notifyChanges();
        }

This else can be refactored to be shared with the onFocusOut handler, but I see no reason for the additional checks - if the grid lost focus prior to end edit, might as well cleanup the active node that editing was blocking.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, okay, so the logic to restore the correct focus is actually baked into the focus out checks :)

if (activeNode.row >= 0 && activeNode.row < this.dataView.length) this.tbody.nativeElement.focus()
if (activeNode.row === -1) this.theadRow.nativeElement.focus();
if (activeNode.row === this.dataView.length) this.tfoot.nativeElement.focus();

eh 😶

this.inputDirective.focus();
// this avoids the grid receiving a focus out because the overlay is removed in the same execution
// chain causing the browser to focus the body element first
setTimeout(() => this.inputDirective.focus());
Copy link
Member

Choose a reason for hiding this comment

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

Kinda sus. The grid receives multiple focus out events, most of which are from in-between focus shifting to the document body. Pretty much every cell click does that and that's why the checks on the handler are so strict about what we want to handle.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, the cell click traced down to ‎IgxCellCrudState.updateCell:

if (cellNode) {
const document = cellNode.getRootNode() as Document | ShadowRoot;
activeElement = document.activeElement as HTMLElement;
activeElement.blur();
}

That blur will force the focus to jump temporarily outside the grid and return, tricking the app scenario usage. This IIRC can be considered a minor bug on the Grid side and it might be better if we avoid blanket blur in general. If replaced with this.grid.tbody.nativeElement.focus() instead it should work fine, though the fix for #14020 should be re-validated.

Copy link
Member

@damyanpetev damyanpetev Oct 16, 2024

Choose a reason for hiding this comment

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

As for the Date Picker - that's another component issue. The entire KB nav of the date picker is broken:
https://www.infragistics.com/products/ignite-ui-angular/angular/components/date-picker#angular-datepicker-example Pick a value, focus does not return to the input.

Edit: Seems to be to do with the calendar stealing back focus after this handler has moved to the input, which causes the closing overlay to dump the focus to body when it's removed from the DOM. If that worked correctly, the focus should switch between the input & calendar - both within the grid container.

Yup, this seems to hit after the calendar has emitted change (also on mouse down for some reason, inner day item) and the picker has started closing and moved focus away:

protected onMouseDown(event: MouseEvent) {
event.stopPropagation();
this.wrapper.nativeElement.focus();
}

Guessing some refactoring happened that changed the internal focus handling of the calendar in a way that didn't occur before and regressed the picker handling.

damyanpetev and others added 2 commits October 18, 2024 19:19
The goal is to blur the internal cell editor to cause a `change` and process
the value, but should not use `blur()` directly as it throws focus on doc body,
rather move the focus elsewhere in the grid and only if it already was there.
@ChronosSF ChronosSF removed the ✅ status: verified Applies to PRs that have passed manual verification label Oct 21, 2024
@ChronosSF ChronosSF assigned ChronosSF and unassigned IMinchev64 Oct 21, 2024
@ChronosSF ChronosSF added the 💥 status: in-test PRs currently being tested label Oct 21, 2024
@damyanpetev damyanpetev force-pushed the sstoychev/focus-changes-editing-m branch from 1c0d4a3 to 835fce9 Compare October 22, 2024 11:18
Copy link
Member

@damyanpetev damyanpetev left a comment

Choose a reason for hiding this comment

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

And we should still add a test for the additional functionality of endEdit - part of it is covered by the usage in the row edit templates that restores focus, but specifically calling after blur still isn't covered

@ChronosSF ChronosSF changed the base branch from master to 18.2.x October 25, 2024 08:49
@ChronosSF ChronosSF added the squash-merge Merge PR with "Squash and Merge" option label Oct 25, 2024
@ChronosSF ChronosSF requested review from damyanpetev and dkamburov and removed request for wnvko October 29, 2024 09:26
dkamburov
dkamburov previously approved these changes Oct 29, 2024
@ChronosSF ChronosSF added ✅ status: verified Applies to PRs that have passed manual verification and removed 💥 status: in-test PRs currently being tested labels Oct 30, 2024
@ChronosSF ChronosSF merged commit 33a554c into 18.2.x Oct 30, 2024
3 checks passed
@ChronosSF ChronosSF deleted the sstoychev/focus-changes-editing-m branch October 30, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

squash-merge Merge PR with "Squash and Merge" option version: 18.2.x ✅ status: verified Applies to PRs that have passed manual verification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Active cell is not cleared if editing is ended on focusout of the grid

4 participants