-
Notifications
You must be signed in to change notification settings - Fork 83
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: focus editor only after it is ready #7235
fix: focus editor only after it is ready #7235
Conversation
@@ -362,5 +364,15 @@ describe('edit column editor type', () => { | |||
editor = column._getEditorComponent(cell); | |||
expect(editor).to.be.not.ok; | |||
}); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New test fails here with Firefox, works locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried the fix manually for both Lit and Polymer, using all supported browsers on MacOS, Windows, and Android. The issue does not seem to be re reproducible in any of them.
@@ -113,7 +113,8 @@ describe('keyboard navigation', () => { | |||
expect(getCellEditor(firstCell)).to.be.not.ok; | |||
}); | |||
|
|||
it('should focus correct editable cell after column reordering', () => { | |||
// See https://github.com/vaadin/web-components/issues/7229 | |||
it.skip('should focus correct editable cell after column reordering', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fails without any change.
dblclick(cell._content); | ||
await nextFrame(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary because of the timing changes.
@@ -180,10 +180,11 @@ describe('edit column renderer', () => { | |||
}; | |||
|
|||
dblclick(cell._content); | |||
expect(spy.calledOnce).to.be.true; | |||
await nextFrame(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary because of the timing changes. Also, the test fails with the existing code when this is added. Focus is always triggered twice.
@@ -194,7 +195,8 @@ describe('edit column renderer', () => { | |||
}; | |||
|
|||
dblclick(cell._content); | |||
expect(spy.calledOnce).to.be.true; | |||
await nextFrame(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
When running tests locally, I get the following failures:
|
Those tests also fail without these changes. They are temporarily disabled in main, and will be restored after the reordering fix is merged. |
I wonder if the PR should be marked with |
const editInitiatorKey = this._editInitiatorKey; | ||
this._editInitiatorKey = undefined; | ||
requestAnimationFrame(() => { | ||
if (editInitiatorKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously it was possible to paste text into a cell, for example with Command + V. When I try this now I only get a v
in the cell editor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just an idea but we could consider redirecting user input to a temporary input element until the editor has finished rendering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach was abandoned. Currently, the Polymer editor is focused synchronously and the Lit editor is focused when it is ready.
This reverts commit 3a62f57.
This reverts commit d5939d1.
Co-authored-by: Sergey Vinogradov <[email protected]>
1280cc1
to
774cb53
Compare
I removed the breaking change signs since the Polymer focus is still synchronous after the latest updates to the fix. |
Co-authored-by: Sergey Vinogradov <[email protected]>
Quality Gate passedIssues Measures |
* fix: defer focus in order to have editor ready before * test: fix unit test for firefox * test: await next render before assertion * test: add another await next render after the first keydown * Revert "test: add another await next render after the first keydown" This reverts commit 3a62f57. * Revert "test: await next render before assertion" This reverts commit d5939d1. * test: update material visual test reference image * fix: set edit initiator char explicitly * test: revert skip test * fix: do not select input when initialized with character * Update packages/grid-pro/test/edit-column-type.common.js Co-authored-by: Sergey Vinogradov <[email protected]> * refactor: extract input selection logic from editor focus * refactor: change name of the new property * refactor: focus lit editor once it is ready * test: revert unnecessary awaits * test: revery unnecessary test assertion changes * Revert "test: update material visual test reference image" This reverts commit f180f2b. * test: skip new test on firefox * test: revert test skip change * Update packages/grid-pro/test/edit-column-type.common.js Co-authored-by: Sergey Vinogradov <[email protected]> --------- Co-authored-by: Sergey Vinogradov <[email protected]>
This ticket/PR has been released with Vaadin 24.4.0.alpha20 and is also targeting the upcoming stable 24.4.0 version. |
Description
The editor for Lit is initialized asynchronously, unlike Polymer. Therefore, during the first focus call, the editor focus element is not ready yet.
This PR
Fixes #6117
Type of change
Checklist