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

[Safari] Images uploads with incorrect size #1975

Closed
Mgsy opened this issue Aug 20, 2019 · 13 comments Β· Fixed by ckeditor/ckeditor5-image#311
Closed

[Safari] Images uploads with incorrect size #1975

Mgsy opened this issue Aug 20, 2019 · 13 comments Β· Fixed by ckeditor/ckeditor5-image#311
Labels
browser:safari type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@Mgsy
Copy link
Member

Mgsy commented Aug 20, 2019

Is this a bug report or feature request? (choose one)

🐞 Bug report

πŸ’» Version of CKEditor

Latest master

πŸ“‹ Steps to reproduce

  1. Upload some bigger image.
  2. If the issue doesn't occur, repeat step 1 few times.

βœ… Expected result

The image is uploaded with a correct size.

❎ Actual result

The image is uploaded with incorrect size.

πŸ“ƒ Other details that might be useful

Screenshot
Screenshot 2019-08-20 at 08 17 06

Tested on Safari 12.1.

@Mgsy Mgsy added type:bug This issue reports a buggy (incorrect) behavior. status:confirmed browser:safari labels Aug 20, 2019
@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2019

Is it a regression?

@Mgsy
Copy link
Member Author

Mgsy commented Aug 20, 2019

It is, can't reproduce it on our demo page.

@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2019

Weird things are happening there :O Looks completely as a Safari bug:

image

That green thing at the bottom is part of the checkmark.

@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2019

Definitely Safari's bug. Toggling any style or resizing the window makes it recalculate all the things and yields different results.

Aug-20-2019 08-52-50

@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2019

From what I can see that's due to display: table that we started using.

@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2019

OK, the workaround that I found is really ugly, but I can't think of something better right now: https://github.com/ckeditor/ckeditor5-image/compare/t/ckeditor5/1975?expand=1

  • If we go this way we should definitely hide it behind an env check to target Safari only.
  • We should also make sure that this code is not executed in unit tests – let's just ignore it there completely because testing it will be a PITA.
  • We can test perhaps some other styles than display. Something less intrusive for the visual appearance. Ideas welcomed :)

@Reinmar Reinmar added this to the iteration 26 milestone Aug 20, 2019
@Mgsy
Copy link
Member Author

Mgsy commented Aug 20, 2019

OK, the workaround that I found is really ugly, but I can't think of something better right now: https://github.com/ckeditor/ckeditor5-image/compare/t/ckeditor5/1975?expand=1

I've tested your solution and it fixes this problem.

@pomek
Copy link
Member

pomek commented Aug 20, 2019

The same issue occurs on mobile Safari (iOS).

oleq added a commit to ckeditor/ckeditor5-image that referenced this issue Aug 20, 2019
@oleq
Copy link
Member

oleq commented Aug 20, 2019

I propose a more subtle solution based on https://stackoverflow.com/a/29804327/1485219. @Mgsy can you check it out?

@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2019

LGTM. Instead of a timeout you force style recalculation by accesing a property. BTW, do we need the last offsetHeight access? Or only the middle one is needed?

@oleq
Copy link
Member

oleq commented Aug 20, 2019

You're right. The second one is obsolete. Updated the branch.

@Mgsy
Copy link
Member Author

Mgsy commented Aug 20, 2019

I propose a more subtle solution based on https://stackoverflow.com/a/29804327/1485219. @Mgsy can you check it out?

Works fine πŸ‘Œ

@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2019

OMFG, I wanted to check how to make this hack pass our tests in Safari, but this turned out to be a pandora's box: https://github.com/ckeditor/ckeditor5-image/issues/312.

Reinmar added a commit to ckeditor/ckeditor5-image that referenced this issue Aug 20, 2019
Fix: Worked around Safari's image size bug. Closes ckeditor/ckeditor5#1975.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser:safari type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants