Support image cropping #439

phenaproxima · 2017-08-24T21:36:43Z

At long, long last: this addresses https://www.drupal.org/node/2690423.

This adds Crop API and Image Widget Crop to Lightning as an optional quasi-dependency of Lightning Media Image. If Image Widget Crop is installed, four crop types are created: freeform, portrait, landscape, and square (thanks @starshaped for suggesting those). Each crop type, and every crop type created thereafter, also gets an accompanying image style which is used to display that particular crop.

In new installs, or after manual updates in existing installs, the Image media type will by default use the cropping widget in both its normal form and in the media browser (which is also used by the image browser). All available crop types will be allowed, but that can be overridden per form display.

@mirodietiker had suggested using Focal Point as our cropping widget, but I decided to stick with Image Widget Crop because it is a more "traditional" form of cropping than Focal Point is, and I figure Lightning shouldn't venture too far off the reservation in terms of expected out-of-the-box CMS behavior. There is nothing in this PR to prevent people from switching to Focal Point if they want to, though.

…et and ImageCropWidget.

…ate.

phenaproxima · 2017-08-24T21:37:26Z

composer.json

@@ -169,7 +173,10 @@
        "drupal/simple_oauth": "2.0.0-rc3",
        "oomphinc/composer-installers-extender": "^1.1",
        "drupal/dropzonejs": "1.0.0-alpha7",
-        "bower-asset/dropzone": "^5.1"
+        "bower-asset/dropzone": "^5.1",
+        "drupal/crop": "1.2.0",


I had to add Crop API as an explicit dependency here in order to patch it. Once the patch is committed, we can remove this dependency, since Image Widget Crop already depends on Crop API.

phenaproxima · 2017-08-24T21:37:49Z

drupal-org.make

 projects[workbench_moderation][type] = module
 projects[workbench_moderation][patch][] = https://www.drupal.org/files/issues/2668006-2.patch
 projects[workbench_moderation][patch][] = https://www.drupal.org/files/issues/2847078-6.patch
 projects[workbench_moderation][patch][] = https://www.drupal.org/files/issues/workbench_moderation-quickedit-support-2749503-6.patch
 projects[workbench_moderation][version] = 1.2
+libraries[cropper][type] = library


This library is required by Image Widget Crop and it's already on drupal.org's library packaging whitelist.

phenaproxima · 2017-08-24T21:43:06Z

modules/lightning_features/lightning_media/lightning_media.module

-function lightning_media_field_widget_info_alter(array &$info) {
-  if (isset($info['entity_browser_entity_reference'])) {
-    Override::pluginClass($info['entity_browser_entity_reference'], EntityReferenceBrowserWidget::class);
+function lightning_media_field_widget_third_party_settings_form(WidgetInterface $widget) {


All of these changes constitute a major refactor of how we were customizing the behavior of image widgets. This is a far more stable way to achieve what we were previously doing with plugin hijacking.

phenaproxima · 2017-08-24T21:43:35Z

modules/lightning_features/lightning_media/lightning_media.module

-      /** @var \Drupal\ckeditor\Ajax\AddStyleSheetCommand $command */
-      $command = new $command_class($query->get('editor'), $style_sheets);
+    if ($style_sheets) {
+      $command = new AddStyleSheetCommand($query->get('editor'), $style_sheets);


This is gardening. Lightning Media has a hard dependency on the CKEditor module, so there was no need to gingerly check anything.

phenaproxima · 2017-08-24T21:44:38Z

...ightning_features/lightning_media/modules/lightning_media_image/lightning_media_image.module

+  if (isset($info['image_widget_crop'])) {
+    // We cannot import the class with a use statement, because it will blow up
+    // if Image Widget Crop is not installed.
+    Override::pluginClass($info['image_widget_crop'], \Drupal\lightning_media_image\Plugin\Field\FieldWidget\ImageCropWidget::class);


To get the behavior I wanted, I had no choice but to hijack the cropping widget plugin.

phenaproxima · 2017-08-24T21:46:02Z

...ghtning_media/modules/lightning_media_image/src/Plugin/Field/FieldWidget/ImageCropWidget.php

+    $value = parent::getSetting($key);
+
+    // If no crop types are chosen, allow all of them.
+    if ($key == 'crop_list' && empty($value)) {


The default behavior of this widget is to only allow crop types which are explicitly enabled for that specific form display. Syncing that with new or deleted crop types would be a complicated pain in the ass from both a DX and UX perspective, so I overrode it. Site builders will still be able to explicitly allow only certain crop types for a particular form display.

phenaproxima · 2017-08-24T21:47:18Z

...tning_features/lightning_media/src/Plugin/Field/FieldWidget/EntityReferenceBrowserWidget.php

-/**
- * Cosmetic enhancements for the Entity Browser entity reference widget.
- */
-class EntityReferenceBrowserWidget extends BaseEntityReferenceBrowserWidget {


This has been replaced by a hook implementation in lightning_media.module, so that we don't need to bother with plugin hijacking.

phenaproxima · 2017-08-24T21:47:45Z

src/LightningExtension/AwaitTrait.php

+use Behat\Mink\Exception\UnsupportedDriverActionException;
+use Drupal\DrupalExtension\Context\MinkContext;
+
+trait AwaitTrait {


This is gardening of the tests. It's a lot nicer than having to pull in MinkContext and call iWaitForAjaxToFinish().

phenaproxima · 2017-08-24T21:48:25Z

src/LightningExtension/Context/UtilityContext.behat.inc

-   * @return \Behat\Mink\Element\NodeElement|null
-   *   The details element, or NULL if it was not found.
-   */
-  public function findCollapsible($summary_text) {


This is replaced by DetailsTrait::assertDetails().

phenaproxima · 2017-08-24T21:49:08Z

tests/config/lightning_api.settings.yml

@@ -1,2 +1,3 @@
 entity_json: true
+bundle_docs: false


Whoops. We should have had this in a previous PR, but I guess we forgot. No harm done.

phenaproxima · 2017-08-24T21:49:53Z

tests/config/views.view.content.yml

-          plugin_id: entity_operations
-        moderation_state:
-          id: moderation_state
+          click_sort_column: target_id


This is the result of a previous PR which moved the operations field. Again, I think we just forgot to merge updated config then. NBD.

phenaproxima · 2017-08-24T21:50:45Z

tests/features/media/image.feature

@@ -16,7 +16,22 @@ Feature: Image media assets
    Then I should be visiting a media entity
    And I should see "Foobaz"

-  @image @javascript @b23435a5
+  @13eacffd
+  Scenario: Cropping should be allowed when creating an image


It'd be really difficult to test the actual cropping, and that's squarely in Image Widget Crop's wheelhouse anyway. I figure it's good enough to ensure that the crop widget is showing up where and when we expect it to.

balsama · 2017-09-05T19:33:58Z

This is excellent and very thorough. Thank you.

I have some questions strictly related to the UI. Some are almost definitely not related to Lightning's implementation, but we should investigate:

I feel like four options out of the box is a little overwhelming. I think the largest use case is an arbitrary crop. Can we only enable "Freeform" out of the box? We can leave them defined, but I think it would be best to enable one type on the Media Image image field at least.
It's not clear to me what the settings at /admin/config/media/crop-widget do. Are they just defaults for new fields that use crop. I tried to modify the settings there thinking it would edit Media: Image before I realized that those settings are on the field itself. Perhaps we could open an issue for Image Widget Crop to clarify what that form does.
/admin/config/media/crop drop-buttons have a link to manage moderation. Is this the case for all entity lists like this? Can we disable it for non-moderated entity types?
The "This crop affects multiple images" warning message seems to be displayed on every image, regardless of whether it's being used or not.

Again, this is great. Thanks so much for this.

phenaproxima · 2017-09-06T14:31:17Z

Fixed. The Image media type will use Freeform only, by default.
I took a quick check and TBH it's not clear to me either. I imagine they'd be treated as the default settings for new fields, but the crop widget itself does not appear to pull default settings from configuration or anything. Maybe it's a vestigial part of Image Widget Crop? In any event, it's an issue with Image Widget Crop, not Lightning. So I'm open to creating an issue (or including a patch) to either document or remove those settings.
I suspect this is an oversight in Workbench Moderation itself. Crop types (config entities) are "bundles" of crops (which are content entities), and I think Workbench Moderation is picking up on that pattern and assuming that moderation is a thing that crop types may want. We can suppress that behavior for crop types specifically, but I would rather whitelist than blacklist. I think we could open a follow-up PR to add an administrative UI to Lightning Workflow which will allow you to say which bundle entity types can have moderation applied. That would solve this problem permanently and generically.
I don't really understand that warning, TBH. There is a setting on the field which can disable that warning, though it's not clear in which contexts. I could set that as the default. What do you think?

balsama · 2017-09-06T14:43:14Z

Cool. Looks like the tests and manual update steps need to be updated (manual update steps documentation and lightning_dev)
Let's open that issue when we get a chance, but not block on it
Let's open that issue against Lighting before merging this
Yes, let's turn off that setting. I'd rather not see that warning at all if possible.

phenaproxima · 2017-09-06T15:01:12Z

Done.
Okay. That sounds like a plan to me.
Opened #447.
The setting in question now defaults to FALSE.

balsama · 2017-09-06T15:03:35Z

Awesome. Let's let this build out to cloud one more time for a quick manual test. Will be here when complete: http://lightningnightlyfpce6h2qzh.devcloud.acquia-sites.com/

balsama · 2017-09-06T17:58:58Z

Shoot. Disabling everything but Freeform causes another UX issue. The others still exist as crop types and each has an associated view mode so the user is still presented with NINE options when embedding. Three of which are identical. (And it's probably not immediately obvious to the user that they could just go enable them on the field)

I like the idea of providing sane defaults, but to keep this as un-intrusive as possible while still providing much needed functionality, let's just ice the config for everything but freeform entirely.

balsama · 2017-09-06T18:36:25Z

Hm. Still see a "Crop thumbnail" image style that I didn't see before this PR (in addition to the "Cropped: Freeform" style that I expected to be there. Fresh install: http://lightningnightlyfpce6h2qzh.devcloud.acquia-sites.com/admin/config/media/image-styles

balsama · 2017-09-07T15:01:28Z

Committed in 0665e16 with the following CI tweaks:

Bumped Travis CI php version to 7.0.22 because symfony/browser-kit requires >=7.0.8 if using php 7.0
Pinned Drupal Console to 1.0.1 because 1.0.2 returns a non-zero exit code for update:execute if there are no pending updates

Adam Hoenich added 28 commits August 21, 2017 14:01

Added Image Widget Crop and the Cropper library.

6e9929e

Added several default crop types as optional config.

d7d85bf

Automatically create an image style for new crop types.

a7c7994

Lightning Dev now removes unique crop effect UUIDs.

4c9da0c

Replaced widescreen crop type with portrait and landscape.

dfabd0a

Mopped up.

26bb2bd

Merge branch '8.x-2.x' into image-cropping

d9f65d6

Merged image_immutable widget back into image_image.

f81f6f9

Created ExtendedImageWidgetTrait, used by our special-sauce ImageWidg…

7bdb7fe

…et and ImageCropWidget.

Minor cleanup, and removed update hook in favor of totally manual upd…

af51d6b

…ate.

Began moving field widget alterations to hook-based structure.

564b812

A bit of clean-up.

789337d

Encapsulated the widget manipulation.

d208d13

A little more clean-up.

f2d4ed3

Opened the cropping area by default.

b029150

Removed a mention of image_immutable.

91d22b5

Trying to work around stupid config schema idiocy.

b02b774

This oughta fix it.

d3eb2ed

Added light test coverage of cropping.

222594c

Fixed fatals and slightly expanded test coverage.

ba997c3

Light refactoring of Lightning Extension.

5497d4a

Minor fixes.

c8fb440

Minor test cleanup.

62b7f45

Fixing the broken.

9dba16b

Patched Crop API to fix a dumb bug.

6d2e4ff

Fixing a stupid typo.

776435e

Added manual update steps, and corresponding update path.

82fc640

Going for the green.

b71c9dc

phenaproxima commented Aug 24, 2017

View reviewed changes

Adam Hoenich added 2 commits September 6, 2017 09:51

Merge branch '8.x-2.x' into image-cropping

6f2f389

Enabled freeform cropping for image media.

394010f

Updated tests and manual update steps, and disabled usage warning.

73a0ea5

Adam Hoenich added 3 commits September 6, 2017 11:21

Okay, maybe this will fix it.

b439ac1

Trying again.

1e4d1ad

Fixing the image_browser.feature test.

d8e5a5d

Ripped out portrait, landscape, and square crop types.

3ff6a6f

Adam Hoenich added 3 commits September 6, 2017 15:27

Default to locally hosted Cropper library, not CDN-hosted.

4904c90

Moved the library resolution code.

95e1af5

Merge branch '8.x-2.x' into image-cropping

734f4c8

balsama closed this Sep 7, 2017

phenaproxima deleted the image-cropping branch September 7, 2017 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support image cropping #439

Support image cropping #439

phenaproxima commented Aug 24, 2017 •

edited

phenaproxima Aug 24, 2017

phenaproxima Aug 24, 2017

phenaproxima Aug 24, 2017

phenaproxima Aug 24, 2017

phenaproxima Aug 24, 2017

phenaproxima Aug 24, 2017 •

edited

phenaproxima Aug 24, 2017

phenaproxima Aug 24, 2017

phenaproxima Aug 24, 2017

phenaproxima Aug 24, 2017

phenaproxima Aug 24, 2017

phenaproxima Aug 24, 2017

balsama commented Sep 5, 2017 •

edited

phenaproxima commented Sep 6, 2017

balsama commented Sep 6, 2017

phenaproxima commented Sep 6, 2017

balsama commented Sep 6, 2017

balsama commented Sep 6, 2017 •

edited

balsama commented Sep 6, 2017

balsama commented Sep 7, 2017

Support image cropping #439

Support image cropping #439

Conversation

phenaproxima commented Aug 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phenaproxima Aug 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balsama commented Sep 5, 2017 • edited

phenaproxima commented Sep 6, 2017

balsama commented Sep 6, 2017

phenaproxima commented Sep 6, 2017

balsama commented Sep 6, 2017

balsama commented Sep 6, 2017 • edited

balsama commented Sep 6, 2017

balsama commented Sep 7, 2017

phenaproxima commented Aug 24, 2017 •

edited

phenaproxima Aug 24, 2017 •

edited

balsama commented Sep 5, 2017 •

edited

balsama commented Sep 6, 2017 •

edited