backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 38 forks source link

[UX] Add an option to "lock" aspect ratio when resizing images in the editor dialog #2054

Open jenlampton opened 7 years ago

jenlampton commented 7 years ago

When resizing images in CKEditor, 99% of the time the user will want the aspect ratio to remain the same. We should make an option to do this (and enable it by default) so that people have a better experience resizing images in Backdrop.

Note that resizing within the editor iframe already maintains aspect ratio, this is just about the aspect ratio within the image dialog.

Graham-72 commented 5 years ago

I think that perhaps this user need is now met with issue #3200.

klonos commented 5 years ago

How do we imagine this to look/work? Should the aspect ratio lock be a checkbox (which allows for a label), or a lockpad icon (which saves on screen real estate)?:

image

This is what the image dialog (WYSIWYG) looks like currently, after having uploaded a new image:

screen shot 2018-10-19 at 10 11 26 am

...and here's what the image field looks like after uploading an image:

screen shot 2018-10-19 at 10 12 06 am

Things to note (do not necessarily need to be solved as part of this ticket here):

  1. The image dialog does not have an image thumbnail.

  2. The image width/height fields are not populated with the image dimensions. These are populated when editing an already uploaded image though:

screen shot 2018-10-19 at 10 15 52 am

...is there a technical limitation, or intentional for some reason?

Graham-72 commented 5 years ago

@klonos these are excellent points you have raised here about the editor's image dialogue and go beyond the original title of this issue. Maybe it should become a new issue that is a review of the image dialogue as we have it now, including use of the image library. Or perhaps we should have a meta issue that is a subset of the CKEditor meta issue #1087 which I find too all-encompassing to get focus on steps we might take for further improvement.

One thing I would like to see added is an indication in the image library of the size of each image for cases where it has two or more versions of the same image.

klonos commented 5 years ago

@Graham-72 sure ...as I said, these do not necessarily need to be solved as part of this ticket here. What I usually do with almost all tickets once they are closed is to go through all past comments and file follow-ups for any ideas or points discussed that were not part of the final solution. It might take me some time, but I eventually get through all/most of them ...and I am happy to see that quite often somebody else has beaten me to it 😄

I honestly do not mean to derail discussions; just logging my thoughts and UI/UX reviews for future reference ...especially during the "needs feedback" status of tickets. I will file follow-up tickets; rest assured 😉

...as for the META, I think one is enough. The way I see metas work is that people can discuss freely as time goes by, and any ideas/plans/tasks eventually end up in the issue summary (which is usually kept up to date by people like myself that have edit permissions in the GitHub queue 😄).

Graham-72 commented 5 years ago

@klonos I'm glad you are there with your encyclopedic knowledge of all issues Backdrop, D7 and D8. Personally, with my less agile brain, I would like to see an issue with a title something like 'Improve WYSIWYG image dialogue', grouping together ideas that exist and arise, and one can focus on as a group. But in the absence of this I will keep my own list in a personal memo. 😃

laryn commented 4 years ago

I just had a specific request for this "Lock proportions" concept in the WYSIWYG editor popup, so +1 from me on this idea. I like the little lock icon and I think that's pretty clear (as long as it fits with Backdrop's UI guidelines -- do we do anything like that elsewhere?).

indigoxela commented 3 years ago

I guess, the "needs more feedback" label can get removed. (Another plus one from me.)

indigoxela commented 1 year ago

Crazy idea: what if we just drop these two number inputs? The editor still provides the "magic corner" to resize the image via mouse drag.

In their current state, they're not very helpful, but can also cause problems (results in squeezed images - depends on the theme).

Reading the comment here after quite some time... Hey when would anyone ever want an image to lose its proper proportions - is there a weird use case for squeezed images I'm not aware of? :laughing:

So dropping the form items is one option, but properly keeping the numbers in sync might be the more bc way.

klonos commented 1 year ago

Crazy idea: what if we just drop these two number inputs? The editor still provides the "magic corner" to resize the image via mouse drag.

Not crazy idea, but does not take into consideration people that cannot use a mouse (either because of disability, or because of hardware restrictions)

...when would anyone ever want an image to lose its proper proportions - is there a weird use case for squeezed images I'm not aware of? 😆

I second that. Once we introduce the aspect ratio lock, I believe that locked should be the default.

indigoxela commented 1 year ago

Not crazy idea, but does not take into consideration people that cannot use a mouse

@klonos you're of course right.

Here's a PR for testing and review. This is, how far I got with my js skills. :wink:

Maybe a "reset to original" button would be a good thing - but I failed to get that working properly. And I also wouldn't know where to put the styles.

olafgrabienski commented 1 year ago

Thanks for the PR! I've had a quick look at the sandbox. Works quite good, but I noticed one issue: When I change one of the dimensions (width or height) directly after uploading the image, the input of the other dimension gets empty, so there is no longer a value in that input. (When I insert the image and edit it again, both values are present.)

Tested in Chrome on Mac.

indigoxela commented 1 year ago

When I change one of the dimensions (width or height) directly after uploading the image, the input of the other dimension gets empty

This may be a matter of msecs - can't reproduce with Chrome on Linux. But I know, that there's a minimal delay as js needs to read the uploaded image.

@olafgrabienski are you able to reproduce reliably, or does this only happen once in a while? Other browser? Other OS (if available)?

olafgrabienski commented 1 year ago

Yes, I can reproduce it reliably, also with Firefox (but no other OS at hand).

In case I didn't explain the issue very well:

indigoxela commented 1 year ago

@olafgrabienski anything in the console log of your browser dev tools?

olafgrabienski commented 1 year ago

Oh yes, Console log directly after changing the first value:

The specified value "NaN" cannot be parsed, or is out of range.
(anonymous) @ js_jj9emzsJm-vHdoXG0tCcmcQC1XMJxAw06One83BLjEQ.js:4
each @ js_jj9emzsJm-vHdoXG0tCcmcQC1XMJxAw06One83BLjEQ.js:2
each @ js_jj9emzsJm-vHdoXG0tCcmcQC1XMJxAw06One83BLjEQ.js:2
val @ js_jj9emzsJm-vHdoXG0tCcmcQC1XMJxAw06One83BLjEQ.js:4
(anonymous) @ js_u_T1_VKlmsWeXowUntSEOaY4quYH_MBrSmEKEPbtfmg.js:1221
dispatch @ js_jj9emzsJm-vHdoXG0tCcmcQC1XMJxAw06One83BLjEQ.js:3
r.handle @ js_jj9emzsJm-vHdoXG0tCcmcQC1XMJxAw06One83BLjEQ.js:3
indigoxela commented 1 year ago

The specified value "NaN" cannot be parsed, or is out of range.

Oops, it's not a number. :thinking: But which step fails...

We should turn off aggregation.

indigoxela commented 1 year ago

With aggregation turned off I can reproduce it in the Tugboat sandbox (but not locally - my "locally" is remote).

Nothing in my console log... But also no number sync.

olafgrabienski commented 1 year ago

In sandbox without aggregation:

The specified value "NaN" cannot be parsed, or is out of range.
(anonymous) @ jquery.js?v=1.12.4:4
each @ jquery.js?v=1.12.4:2
each @ jquery.js?v=1.12.4:2
val @ jquery.js?v=1.12.4:4
(anonymous) @ filter.js?v=1.24.x-dev:384
dispatch @ jquery.js?v=1.12.4:3
r.handle @ jquery.js?v=1.12.4:3
indigoxela commented 1 year ago

I think I found a way to reproduce reliably - and it seems to be a logic problem. It's attempted to get the image dimensions before the "onload" of the image has succeeded. Have to think this over... :thinking:

More testing is still welcome!

indigoxela commented 1 year ago

I think I found a way to deal with the asynchronous image loading. Not sure if it's the most elegant solution (but reading all that promise/resolve/then stuff again caused me headache :stuck_out_tongue_winking_eye:).

Further testing would be cool!

olafgrabienski commented 1 year ago

Further testing would be cool

Great, works for me in the sandbox!

argiepiano commented 1 year ago

I have a question about this. I'm not sure if I'm missing something, but I don't see the option to lock/unlock the aspect ratio. As far as I can tell, this PR effectively disables the ability to change the aspect ratio/proportions of an image. I admit this is unusual, but I have used images in that past where the aspect ratio was not essential (non-photos for example), and the ability of changing only the height but not the width came handy.

There is some conversation above adding a lock. Is that still being considered for this PR?

argiepiano commented 1 year ago

@indigoxela thanks again. I left some comments.

docwilmot commented 1 year ago

I have a question about this. I'm not sure if I'm missing something, but I don't see the option to lock/unlock the aspect ratio

I have been in the situation where I have an image that is already skewed in its original state and one way to fix that is to stretch one dimension to make it look passable. I think we need an option to unlock.

indigoxela commented 1 year ago

I'm not sure if I'm missing something, but I don't see the option to lock/unlock the aspect ratio.

That's based on the previous discussion here - who would ever want a squeezed image?

I have been in the situation where I have an image that is already skewed in its original state and one way to fix that is to stretch one dimension to make it look passable. I think we need an option to unlock.

Bare with me, this may take some time. I'm working on the edges of my js skills already. Further tweaks and additional features might completely be out of reach for me. So if someone with better js skills wants to take over here - feel free to do so.

If we need buttons - where should the styles go (they will require styling)? To the filter module, to system css, or to the themes. I'm aware that this discussion will block progress here for a while.

indigoxela commented 1 year ago

Let's see if we can revive this issue.

@docwilmot wants an option (button) to unlock - and that's something I don't want at all.

We could...

Would either of that bring us a step forward? Feedback is welcome!

BTW: looking at the current issue label - I don't think that "task" is still the right one, it's rather "feature". IMO the change isn't small enough for a task anymore.

olafgrabienski commented 1 year ago

I could provide this functionality as contrib module

Are you speaking about the lock functionality or about the unlock option?

indigoxela commented 1 year ago

Are you speaking about the lock functionality...

The lock functionality, of course. When it's in contrib, it's optional, anyway.

If in core, then with a newly added on/off setting per filter format via admin UI (not per image).

olafgrabienski commented 1 year ago

The lock functionality, of course

Thanks for the feedback! I guess that wasn't obvious to me because the lock functionality was discussed as a 99% use case (while the unlock option was mentioned as rather unusual, even by people who asked for it). So, I'm still for putting the lock functionality in core. And yes, an unlock setting may be a good compromise.

argiepiano commented 1 year ago

@docwilmot wants an option (button) to unlock - and that's something I don't want at all.

FWIW I also support the inclusion of the lock/unlock button. This is something that's standard in pretty much every image UI I've ever used (granted, I'm no expert). It seems strange that this is not included here, and the rationale for not including it seems flawed (b/c it's hard?)

D7 CKEditor:

Screen Shot 2022-11-15 at 6 42 44 AM
indigoxela commented 1 year ago

FWIW I also support the inclusion of the lock/unlock button

Hm. Besides that personally, I don't want that feature (would want to turn it off, if it existed), my concern is the "per image" thing, which probably bloats the code just for an edge case (it needs more checks). And probably is unreliable to handle.

In that case the only compromise I see, is contrib.

indigoxela commented 1 year ago

Until we have progress here, there's now a contrib module: Editor Image Dimension Sync

indigoxela commented 1 year ago

The PR is still open, but I won't work on it anymore, so feel free to take over (anyone).

laryn commented 1 year ago

I would support the locked functionality as default on new installs, with an option to globally turn off the locking in the rare cases that someone wants to be able to do that.

olafgrabienski commented 1 year ago

with an option to globally turn off the locking in the rare cases that someone wants to be able to do that

Hm, I guess most people who need the unlocking option don't need it all the time. In other words: most people who want the unlocking option, do need it only in certain cases. And in all other cases they want to benefit from the locking functionality. So in my opinion, a global setting shouldn't turn off the locking functionality, but turn on the unlocking option. (Hope that makes sense!)

indigoxela commented 1 year ago

So in my opinion, a global setting shouldn't turn off the locking functionality, but turn on the unlocking option.So in my opinion, a global setting shouldn't turn off the locking functionality, but turn on the unlocking option.

As long as I can turn it off globally, I'm fine with it.

I still belief, squeezed images aren't useful at all. The use case @docwilmot describes indicates a broken image - which could get fixed by uploading a fixed version. That's easy to achieve with managed files.

Giving people the ability to turn off locking (by default or by image) has other disadvantages: when opening that node form again, the editor will show the image in the original aspect ratio, using the grippie-corner for resizing (not sure what the name is) will always enforce locking, so this workaround for broken images is very fragile.

@argiepiano I'm aware that the CKEditor image plugin in D7 had that unlock (anti-)feature, but in D7 the editor shows the image squeezed - at least in themes without support for responsive images like Seven. So people in D7 see what they get, in B that's not the case. In the editor they see the original aspect ratio (because of "height: auto"), but when they save the node, the image is squeezed again (a typical WTF).

indigoxela commented 1 year ago

Some months later we probably all look at the topic and previous discussions with a fresh mind. Time for a rebase. :wink:

Will look at the possibilities for a compromise later, just wanted to give people the chance to try things out in a fresh sandbox.

indigoxela commented 1 month ago

PR removed. :wink: So this issue's open for new approaches. Until then, there's this contrib module for the dimension sync.