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

Remove width and height after quit editing #1899

Merged
merged 4 commits into from
Jun 20, 2023

Conversation

juliaroldi
Copy link
Contributor

@juliaroldi juliaroldi commented Jun 19, 2023

Only set image width and height to the image in editing mode. After editing the image do set width and height style properties. This properties were removed, because it was causing an issue in Outlook after the image has been sent.

Bug:
Screenshot 2023-06-16 at 14 37 18

Expected:
Screenshot 2023-06-16 at 14 37 00

@juliaroldi juliaroldi marked this pull request as ready for review June 19, 2023 20:11
@juliaroldi juliaroldi merged commit 31f2fb7 into master Jun 20, 2023
@juliaroldi juliaroldi deleted the u/juliaroldi/editting-height-width branch June 20, 2023 14:00
@haven2world
Copy link
Contributor

haven2world commented Jul 17, 2023

Hi @julia, this change made a regression on outlook mobile side. When we edit an image with a preset "width" style, which has a higher priority than element's "width" attribute, if we only apply the change on element's "width" attribute, then the image's size won't change.

@juliaroldi
Copy link
Contributor Author

Hi @julia, this change made a regression on outlook mobile side. When we edit an image with a preset "width" style, which has a higher priority than element's "width" attribute, if we only apply the change on element's "width" attribute, then the image's size won't change.

Hey @haven2world, we made this change to fix a issue in client web. Do you need to keep image width in the Outlook mobile scenario?

@haven2world
Copy link
Contributor

I read the PR description. I just wonder whether an image like <img src="foo" style="width: 300px"> can still be resized by percentage after this change. Because after upgrading roosterJS, one test case like this failed on our end.
The cause is after this change, the new width only apply on the width attribute, for example, the user want to change its width to 200px, then the element is changed to <img src="foo" style="width: 300px" width="200">. But the width style still has a higher priority than attribute, so the image's width will still be 300px

@juliaroldi
Copy link
Contributor Author

I can reproduce the issue you are reporting now. We should not have width/height style attributes in the final image. If the image has this style attributes, we should remove it and only let width/height attributes.

@juliaroldi
Copy link
Contributor Author

Hey @haven2world we have a fix for the issue you reported #1961. Do you need a patch fix for it or is it ok to wait for the next release?

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 this pull request may close these issues.

3 participants