OfficeDev / office-js

A repo and NPM package for Office.js, corresponding to a copy of what gets published to the official "evergreen" Office.js CDN, at https://appsforoffice.microsoft.com/lib/1/hosted/office.js.
https://learn.microsoft.com/javascript/api/overview
Other
690 stars 95 forks source link

Undo stack clears after using Office.context.document.setSelectedDataAsync("Text to insert") #3985

Open bo-ravnborg opened 10 months ago

bo-ravnborg commented 10 months ago

Your Environment

Expected behavior

When using Office.context.document.setSelectedDataAsync("Text to insert") I expect the text to be inserted and whatever was written in the same textbox before the insertion can still be undone.

Current behavior

When inserting the text to replace the selected text in a text box, the undo stack clears and you cannot undo anything changed in that textbox before the insertion. You can still undo the insertion, but not anything before that. If you undo and redo it will also not redo the inserted text, but instead just a blank text.

Steps to reproduce

  1. Insert a text box in a PowerPoint slide.
  2. Write multiple lines of text in the textbox
  3. Without losing focus of the textbox, select one of the lines of text
  4. Replace text by executing the function: Office.context.document.setSelectedDataAsync("Text to insert")
  5. Undo as much as possible. It's only possible to undo the insertion, and when redoing it only inserts a blank text.

Context

I am trying to make an add-in that reads selected text, modifies it according to the user action, and replaces the selection with the new text. It is the last part, regarding the replacement of the selected text, that is causing this issue.

xuruiyao-msft commented 10 months ago

@bo-ravnborg Thanks for reaching out to us. I can randomly reproduce the issue according to the steps you provided. The key point to reproduce it is operating ppt with latency. If you type words in the normal or fast speed, the redo/undo combines the several input/replace actions into one action, then if you click redo/undo, several lines appear/disappear together, which much like the below video shows:

https://github.com/OfficeDev/office-js/assets/5762929/ef79e018-0006-405d-81a9-3ef0d2bd5cde

I try to operate PPT with latency, after I undo all the actions, I cannot redo to recover to the original state:

https://github.com/OfficeDev/office-js/assets/5762929/b10b9839-a541-410f-9065-a411f5b880ef

Would you please help confirm if this behavior is what you described? Thanks.

bo-ravnborg commented 10 months ago

@xuruiyao-msft Thanks for looking into this! Testing a bit more, it seems it's actually the getting of selected text that is messing up the undo-stack. When testing previously I was still getting the selected text in the background, so that was why I was seeing the issue.

The way I'm getting the selected text is like this:

public static async getSelectedTextInPowerPoint() {
  return PowerPoint.run(async (context) => {
    const selection = context.presentation.getSelectedTextRange();
    context.load(selection, 'text');
    await context.sync();
    return selection.text;
  }).catch((error) => {
    return '';
  });
}

Doing this seems to completely remove the undo-stack for the current text box. See this example:

https://github.com/OfficeDev/office-js/assets/68271198/b46e2577-78a1-4d61-8449-0918f27e8166

I tried a different approach and that seems to not give me the same issue:

public static getSelectedTextInPowerPoint(): Promise<string> {
  return new Promise<string>((resolve, reject) => {
    Office.context.document.getSelectedDataAsync(
      Office.CoercionType.Text,
      (asyncResult) => {
        if (asyncResult.status === Office.AsyncResultStatus.Succeeded) {
          // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
          const text = (asyncResult.value ?? '') as string;
          resolve(text);
        } else {
          reject(asyncResult.error);
        }
      },
    );
  });
}

But it does look like there is still an issue redoing the change as you also see in your example. Regarding the issue related to context.presentation.getSelectedTextRange(). Should I rename the issue or make another one? Or am I using this function incorrectly?

EsterBergen commented 10 months ago

Hi @bo-ravnborg - Thank you for submitting this! We'll look into it and get back to you shortly.