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.

Sign up for GitHub

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

Replace first cell content if input while on cell selection #2030

Merged
merged 7 commits into from
Aug 17, 2023
Merged

Replace first cell content if input while on cell selection #2030

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
20c86c2
select first cell and empty
Andres-CT98 Aug 15, 2023
cd3c3cb
Select cell content
Andres-CT98 Aug 16, 2023
ab657cb
Undo Snapshot
Andres-CT98 Aug 16, 2023
75c4cde
remove undo
Andres-CT98 Aug 17, 2023
ce2b15e
Merge branch 'master' of https://github.com/microsoft/roosterjs into …
Andres-CT98 Aug 17, 2023
699fd15
fix undo
Andres-CT98 Aug 17, 2023
257ad98
fix dependency
Andres-CT98 Aug 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions packages/roosterjs-editor-plugins/lib/plugins/TableCellSelection/TableCellSelection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,13 @@ export default class TableCellSelection implements EditorPlugin {
event.rawEvent.preventDefault();
}
break;
case PluginEventType.Input:
if (!this.state.startedSelection) {
this.handleInputEvent(this.editor);
} else {
event.rawEvent.preventDefault();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is Input event preventable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems we just need to handle keydown event, it will be good enough. I tried to comment out the handling code of input event, and it still works well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an issue with undo that is mitigated by using the Input method. It only happens if you type one character and do undo, the inputted character and the last written thing will both be undone. This issue is present today and not introduced by this change.

The reason I clear the full cell is that I do not know if select is enough to cover all possible elements in the cell for deletion. And for clearing I want to make sure it is an input and not other keys like caps lock.

Copy link
Collaborator

Choose a reason for hiding this comment

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

About the undo issue, can you provide a full end to end repro step? I want to understand more about it. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Create a table
  2. Fill it with values, remembering which one was the last one
  3. Make a cell selection
  4. Type one character
  5. Undo

Notice that the typed character and the last filled value are both undone. The only one undone should be the typed one.
tableUndoWrong

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, in that case we do need to call addUndoSnapshot() when type with table selection, but only need to do once.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And I think it should be good enough to just select everything inside the table cell when keydown. Because:

  1. User made a table selection, then type
  2. User selected everything in a table cell, then type

Both of them should get the same result. So making a selection to the first select cell means we convert 2 to 1, so that is the right operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have been trying both selection types, but each have an issue. editor.select with the Td as parameter has the above issue, while editor.select with the table and coordinates as parameter has an issue of replacing everything in the cell when typing. For example if the cell has multiple lines, only the first one is replaced.

}
break;
case PluginEventType.Scroll:
if (this.state.startedSelection) {
handleScrollEvent(this.state, this.editor);
Expand Down Expand Up @@ -127,4 +134,18 @@ export default class TableCellSelection implements EditorPlugin {
editor.select(selection.table, null);
}
}

private handleInputEvent(editor: IEditor) {
// If typing on any cell selection, clear the first cell
const range = editor.getSelectionRangeEx();
if (range.type == SelectionRangeTypes.TableSelection) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, when I debug your code, Input event happens after KeyDown event. Since you have already changed selection when KeyDown event, it will fail the check of table selection range type, so will go directly to line 149, so code from line 142 to line 146 is never executed.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. I assumed it would retain the selection type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because you have called editor.select(), that will change selection type.

Copy link
Collaborator

@JiuqingSong JiuqingSong Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

And because of that, the executing result is correct now. But if line 142-146 are executed, the new typed content will be removed, just because when Input event is triggered, the new typed content is already added into the view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have noticed that, would it work if I did the deletion before the Input event? I would need to add it to PluginEventType.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My feeling is you just need to keep you current code to select the content of first selected cell, (and call addUndoSnapshot() in a proper place), then let browser handle everything else, so no need to handle Input event at all.

Because Input event is triggered after type, so anything there is too late.

And maybe only need to handle character types (you can use isCharacterValue function to check) so for other keys like caps lock, leave it to browser

const row = range.ranges[0];
const firstCell = row.startContainer.childNodes[row.startOffset];
firstCell.childNodes.forEach(node => {
node.remove();
});
}
// Add undo snapshot after content deletion
editor.addUndoSnapshot();
}
Andres-CT98 marked this conversation as resolved.
Show resolved Hide resolved
}
8 changes: 7 additions & 1 deletion ...es/roosterjs-editor-plugins/lib/plugins/TableCellSelection/keyUtils/handleKeyDownEvent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,13 @@ export function handleKeyDownEvent(
editor.getSelectionRangeEx()?.type == SelectionRangeTypes.TableSelection &&
(!isCtrlOrMetaPressed(event.rawEvent) || which == Keys.HOME || which == Keys.END)
) {
editor.select(null);
// Collapse selection to first selected cell
const range = editor.getSelectionRangeEx();
if (range.type == SelectionRangeTypes.TableSelection) {
Andres-CT98 marked this conversation as resolved.
Show resolved Hide resolved
const row = range.ranges[0];
const firstCell = row.startContainer.childNodes[row.startOffset];
editor.select(firstCell);
}
}
}

Expand Down