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.

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

selectImage will overwrite the id of the image element #1467

Closed
milliezhang-qinlan opened this issue Dec 16, 2022 · 10 comments · Fixed by #1480
Closed

selectImage will overwrite the id of the image element #1467

milliezhang-qinlan opened this issue Dec 16, 2022 · 10 comments · Fixed by #1480
Assignees

Comments

@milliezhang-qinlan
Copy link
Contributor

Outlook Mobile is using the image resize feature in roosterjs, while after ingesting the latest roosterjs version, we find some updates that might affect our feature functionality.

We find when triggering ImageEdit.setEditingImage, the image being resized will lost its original id and the id would be replaced by the ones starting with "imageSelected" prefix. It looks like it is due to the selectImage function, which would trigger addUniqueId and thus replace the image's original id. Since we somehow rely on the image element id, replacing it would introduce some unexpected issues.

Is the replacing of the selected image element's id an expected behavior? And why are we doing this? And is there a way to skip the replacing of id if the id is already there?

To Reproduce
Steps to reproduce the behavior:

  1. Inserting image in the roosterjs demo and add an id to the image element.
  2. Click the image to activate the editing mode
  3. Check the image element's id and it would be replaced with id starting with "imageSelected" prefix
  4. Exiting the image editing mode, the image element's id is in "imageSelected" format.

Expected behavior
The image's original id shall not be overwritten.

Screenshots
If applicable, add screenshots to help explain your problem.

Device Information

  • Browser: Chrome
@JiuqingSong
Copy link
Collaborator

@juliaroldi would you please take a look

@juliaroldi juliaroldi self-assigned this Dec 19, 2022
@milliezhang-qinlan
Copy link
Contributor Author

@juliaroldi Since this issue is blocking us from ingesting latest roosterjs version which contains several bug fixes, I am wondering if there is an estimated time when this issue can be resolved? Thanks!

@juliaroldi juliaroldi linked a pull request Jan 4, 2023 that will close this issue
@juliaroldi
Copy link
Contributor

Hey @milliezhang-qinlan, I'm working on fix and I hope to ship it in the next rooster version.

@juliaroldi
Copy link
Contributor

juliaroldi commented Jan 4, 2023

Hey @milliezhang-qinlan, if you insert two images, is it possible to have the same id for both? Because the id will only be replaced, if it has more than one. If the id is unique, this PR #1480 will fix the issue.

@juliaroldi juliaroldi linked a pull request Jan 4, 2023 that will close this issue
@milliezhang-qinlan
Copy link
Contributor Author

milliezhang-qinlan commented Jan 6, 2023

Hey @milliezhang-qinlan, if you insert two images, is it possible to have the same id for both? Because the id will only be replaced, if it has more than one. If the id is unique, this PR #1480 will fix the issue.

@juliaroldi Actually for the normal case of inserting an image, we would try to apply a unique id. But how about copying and pasting the existing images ? The copied images would have the same id.

@juliaroldi
Copy link
Contributor

@milliezhang-qinlan When we paste the image, the id is removed. Do you need the pasted image id to be same of original one?

@milliezhang-qinlan
Copy link
Contributor Author

@milliezhang-qinlan When we paste the image, the id is removed. Do you need the pasted image id to be same of original one?

Thanks @juliaroldi for the info! Could you point to the code where the id will be removed on pasting the image? I think we did not expect the id of the pasted image to be removed. And could you explain the reason to do so? Thanks!

@JiuqingSong
Copy link
Collaborator

Id should be unique. If paste with the same id, it means there are more than one elements have the same id, which is not unique anymore. This is not a new behavior, but has been there for years.

On the other hand, the reason why you need an Id is because you have some code to insert some content and you want to be able to get the inserted content using its id. However, after copy/paste, the pasted image is inserted by user's "paste" operation, but not your code. So it is expected to not be able to get the pasted image using id, because it is already "user's content" now rather than your content.

@milliezhang-qinlan
Copy link
Contributor Author

Thanks @JiuqingSong for the explanation. It makes sense.

@milliezhang-qinlan
Copy link
Contributor Author

this PR #1480 will fix the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants