unable to prevent cell editing in event onBeforeEditCell

b-giavotto commented 9 months ago

Describe the bug

there is no way to prevent cellediting (returning false on the event subscriber) in beforeEditCell handler of the grid.

Reproduction

define and subscrive an event on grid onBeforeEditCell, in the body simply return false; the cause is because of the protected method called in makeActiveCellEditable does a call to another method, named trigger: if (this.trigger(this.onBeforeEditCell, { row: this.activeRow, cell: this.activeCell, item, column: columnDef, target: 'grid' }).getReturnValue() === false) {

But the test is wrong!: the return value IS not a boolean type but instead a Promise I think makeActiveCellEditable is called on other sites, and the fix should affect even others parts.

Which Framework are you using?

Vanilla / Plain JS

Environment Info

| Executable          | Version |
| ------------------- | ------- |
| (framework used)    | DNA|
| SlickGrid | VERSION |DNA
| TypeScript          | DNA|
| Browser(s)          | DNA|
| System OS           | DNA|

Validations

[X] Follow our Code of Conduct
[X] Read the Wikis.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Check that this is a concrete bug. For Q&A open a GitHub Discussion.
[X] The provided reproduction is a minimal reproducible example of the bug.

6pac commented 9 months ago

I'll leave this one to @ghiscoding , I think it's probably a typescript issue.

ghiscoding commented 9 months ago

I don't have any problems with onBeforeEditCell and it works as intended on my side, please provide code to replicate the issue or contribute a Pull Request

ghiscoding commented 9 months ago

The comments seems invalid, also since we did not have any examples with onBeforeEditCell returning false, I modified one of the example in PR #993 with proper E2E tests validating the effect of returning false within the subscriber. Another alternative might be to call e.preventDefault() to cancel event bubbling

But the test is wrong!: the return value IS not a boolean type but instead a Promise

this is wrong, it doesn't return a Promise but rather a SlickEventData which is synchronous and the way to get the returned value (however this is usually meant to be used by internal code only) is to call .getReturnValue() like we do internally in SlickGrid

https://github.com/6pac/SlickGrid/blob/61e73db0027d10714a8b4ca8f19f4ccb37269301/src/slick.grid.ts#L5926-L5929

So I'm closing since the details seems incorrect and there was also no feedback provided when asking for a repro

6pac / SlickGrid