Refactor form element and JS to work with any theme

hi @dkre,
This issue is smally linked about #2692051: [META] Alternative UI like divibuilder issue. At the beginin of module IWC have an specific structure and purpose a theme not standard in Drupal context. This is take a few problems for integrate IWC in Media ecosystems. After long discuss we have purpose an standardized UX for the module and decided to integrate IWC with existant elements on Drupal.

Vertical Tabs are the best choose in this context (like standards) and permit to listen events per example (i can invite you to read #2625026: Crop widget crop list not following common patterns for understand all efforts / choose about standardization of IWC UX.

BUT !! Your needed are a true issue ! We need to permit user of IWC to use a "standard" theme or permit to extend our elements and customize free the ux of IWC to render it more "sexy" but we need to preserve critical elements needed by IWC libraries.

I am free to talk about it in #drupal-media or imagewidgetcrop.slack.com !

+1

We are using Bootstrap, and the cropping doesn't work as expected (no limited crop ratio). Also, the cropping box doesn't appear at all upon opening the "Crop Image" option--you must first click "Reset crop" to get the box to appear (without the proper fixed ration).

hi,

IMHO it's not really for IWC to assume that task. IWC have already proceded an refactor to "standardize" all elements to have an drupal friendly interface. We need to create an issue onto Boostrap theme project to identify (or in this issue) the real problems and decide to assume the patch on boostrap or IWC patch.

Thanks @woprrr and @dkre. If you end up opening an issue for this on Bootstrap, please link it here.

IWC have already proceded an refactor to "standardize" all elements to have an drupal friendly interface.

it's just not right a single popular theme can skew the standards

What "standards" are you referring to exactly?

Themes can (and do) change markup completely. Including, up to, removing or changing certain elements and classes. It's the job of the module to structure its JS so it's more abstract.

If the "standards" referred to here are based on core themes (e.g. bartik, seven, etc.) then that is not a "standard". That is just getting it to work with default output. These core themes should never be touted as a reliable "test" on whether or not something works with other contrib.

The true "standard" is not for a module to rely on classes spit out by core, but rather add its own (JS) classes, or more accurately, data attributes to use as selectors in JS. This will ensure that it works in any theme, regardless of the changes that were made in the theme.

A module should never rely on specific elements or classes it does not control.

The problem isn't the base theme.

is there any news on the development? Bootstrap is currently the most used theme, I think at least you should make the form used for this

someone still maintains this module?

Yes, but ATM this issue is not major or problematic. I need more time to write an correct response.

If you have an idea or purpose a patch we are free and that can be a good begin of solution.

to bootstrap I partially solved by overwriting the JS in this way

libraries-override:
  image_widget_crop/cropper.integration:
    js:
      js/imageWidgetCrop.js: js/image_widget_crop/imageWidgetCrop.js

and the JavaScript:
http://pastebin.com/QL8LLFVi

However I can not get activate vertical tab without having to click on it

can someone help me at least understand how to select the first item automatically because the initialize is not working?

@sedax for that part I'am a little out, these Front part is not my best friend :( I've already ping pivica to have an expert eyes of problem. I think this problem is smally linked to https://www.drupal.org/node/2660786#comment-11823602 (we need a new refactor of this part).

I switch that major because we need to found an robust solution fast. I try to have time to this issue as soon as possible.

@markcarver Now I've more time to respond you and explain our problems.

What "standards" are you referring to exactly?

Themes can (and do) change markup completely. Including, up to, removing or changing certain elements and classes. It's the job of the module to structure its JS so it's more abstract.

If the "standards" referred to here are based on core themes (e.g. bartik, seven, etc.) then that is not a "standard". That is just getting it to work with default output. These core themes should never be touted as a reliable "test" on whether or not something works with other contrib.

Yes, and it's my opinion and at the begining IWC have our proper markup based on proper classes / attributes but that have change to refactor all elements to "more standard markup" to have a better integration with Drupal UI. @see #2625026: Crop widget crop list not following common patterns to have historic.

Then now I am really open to permit boostrap integration with IWC and we DO found an solution to solve it. Actually Adminimal theme work with IWC, I think our problem are the total rewrite of details / vertical_tab (.vertical-tabs__menu-item) element due these bug. I think our problem are not really critical because if we click on "reset" cropper work (a bit destroyed...) but these work. What you suggest me to permit you have a better integration with IWC ?

@sedax I see your patch and take a feedback these afternoon.

That's a start of solution to purpose anything ...

I describe more explicitly a proplems

1 : We Use an 'classy' class "details-wrapper" to identify the image to crop area and with boostrap that is not used that is re-writte by "bootstrap-panel.html.twig"
In these case we do change the $cropWrapperSummary into js file to return alway the real wrapper without usage of details-wrapper or summary element. I'm fully open to purpose O_O

2 - For each element we use "vertical-tabs-panes" provide by vertical_tab core form element. To permit other themes to works with us I have add 'crop-tabs__menu-panes' class in same level to avoid problems.

I need some time / suggestion to permit all theme to work with IWC. In my thought :

1 : To avoid all problems with "details" create another element herited by details like "crop_wrapper" to permit IWC to inform themes we need a certain type of structure.

2 : Refactor a part of js but that is out of my scope actually, (if we can purpose anything I take !).

1 : To avoid all problems with "details" create another element herited by details like "crop_wrapper" to permit IWC to inform themes we need a certain type of structure.

No, that isn't necessary. Just add a unique data attribute:
$element['#attributes']['data-drupal-iwc'] = 'wrapper'

Then it can be targeted in JS:
var $wrapper = $context.find('[data-drupal-iwc=wrapper]');

This is how you should target everything in JS, it's not specific to classes, elements, themes or whatever else... because it's unique to this module.

No, that isn't necessary. Just add a unique data attribute:
$element['#attributes']['data-drupal-iwc'] = 'wrapper'

Actually, in these level we have already add "image-data__crop-wrapper" to identify the wrapper element but actually we use 'details-wrapper', I agree that is an "classy" base_theme class and THIS IS BAD ! It's easy to use ".image-data__crop-wrapper > div:first-child" instead to avoid this problem.

Now another problem with ".vertical-tabs__menu-item" that is define by JS in Drupal.theme.verticalTabs() and we have any ways to override / change it in IWC level to identify element in vertical_tabs elements. Instead use
var $verticalTabsMenuItem = $verticalTabs.find(verticalTabsMenuItemSelector); we should use var $verticalTabsMenuItem = $verticalTabs.find('.vertical-tabs > ul:first-child > li'); but these are possible to have another 'ul' element in future and we haven't way to be sure we seleted the good element.

I purpose that patch to solve the situation, for me that save all functionalities of IWC / Boostrap and permit to work well.

WTF test pass and that flag not pass*_* retry this !

The patch #18 seems to work well, I'll do the tests in one day but it seems ok

Tested during Media sprint at Davos ;). Thanks all guys !

This module's JS code does not follow the established JavaScript and jQuery coding standards very well.

Furthermore, the constant use of > and multiple selectors in one jQuery object instantiation isn't considered "Best Practice™", primarily for performance reasons (see http://stackoverflow.com/q/3177763/1226717).

Thanks @markcarver to your feedback about that ! I admit the JS part isn't my "speciality" but that isn't the current subject in this major issue. The priority are unblock the situation with boostrap to permit us to work well. Can you create an issue to explain more your analize about module ? I really need more feedback about these part ! If you can't or haven't time to create it tomorrow I create it and that would be great if we can add your opinion.

that isn't the current subject in this major issue

Yes it is. "refactor" !== "just fix", it means to completely redo the code to be more abstract and universal, following existing "Best Practices™" that are clearly documented through out the community.

The above patch is expanding the bloat of the, already, poorly constructed JS selectors/code.

Can you create an issue to explain more your analize about module ?

That is what this issue is.

I really need more feedback about these part !

I have given feedback. Lots of it.

Hi @markcarver,

I understand totally your objective and I already thank you for you precious feedback. You do understand too my approach in general I really hate long/tunnel effect due by big changes in issue. That is symptomatic of D.O to have verry big change in issue that indeed more complicated recette phase and more chance to forget features looses or bug introduced.

My approach is more atomic, I really like to fixe problems step by step. In this issue you have pointed with @sedax two problems, one MAJOR bug incompatibility with boostrap theme specifically (adminimal/rubic or other already works with IWC). And another important task to REFACTOR ALL JS to respect standards js / Jquery / Drupal etc...

The most important for me is the first part to permit users to use boostrap & image_widget_crop now ! and this is why I have already merge the first part in dev branch. The second part can be continued in this issue or I think we can create another issue to resume all thinks and re-start better in these subject with people intersted by JS part.

This is not to disturb you just for organize better the fixes workflow of module and to bo more itterative.

What you think about it ? We continue here or I create another issue with resume of your idea / feedback / directive to have a better module :) I do my best to manage this module well as possible do not be annoying.

I understand totally your objective

No, not really.

You do understand too my approach in general I really hate long/tunnel effect due by big changes in issue. That is symptomatic of D.O to have verry big change in issue that indeed more complicated recette phase and more chance to forget features looses or bug introduced.

I understand the hesitation, sure. However, refactoring anything usually tends to be rather large by nature. There's usually no way to get around that. Implementing "workarounds" usually only ever come back to haunt...

My approach is more atomic, I really like to fixe problems step by step.

Except that you didn't really "fix" anything at all. You just committed a "temporary fix" that is specific to only one theme (e.g. there were Bootstrap specific selectors added).

As I already stated above, many times, the code needs to be entirely abstracted so it can work with any theme.

This issue was never about Bootstrap specifically. The fact that it "didn't work" in Bootstrap was merely just a symptom of a much larger problem: poorly constructed JS and jQuery selectors.

I will not continue trying to stress the need for better standards of code quality; it's clear you're the maintainer here and will do whatever you want.

@markcarver: I agree that the js should be refactored to use data selectors set by the module itself instead of classnames provided by core/classy/whatever.

The vertical tabs code seems to be a special case here, which is created by javascript (in /core/misc/vertical-tabs.js)
Bootstrap overrides this javascript to create its own structure for vertical tabs.
This makes injecting custom data selectors pretty hard for those items.

We should investigate if we somehow can target the necessary items with custom data selectors for this case or if verticaltabs has custom events which we can hook in to, instead of using a css selector.

Patch attached refactors the current selectors,not defined by vertical-tabs.js, to use data attributes instead.

It does not remove or change existing css classes.

#29 still applies, and support for themes/modules overriding vertical-tabs.js theme function still requires special care.

Did some testing on this.

This patch does fix the issue in the Bootstrap theme so that the cropping widget scales properly, and activates upon opening the crop tool, however...

There are issues when there is more than one image field with a cropper on the page.

Clicking on any "Crop image" fieldsets will open ALL of the "Crop image" fieldsets on the page. However, only the "Crop image" you clicked on will be activated, and... the fieldsets cannot be closed after being opened.

Hi guys I'm glad to see you on these issue :) thank a lot @StryKaizer I see your patch this week thank a lot ! Marcaver have reason but out ways to achieve this are not the same. Now I am focused on that issue to transform the js as better as possible. StryKaizer Yes the structure of vericale tabs are a little spécial and events with this élément too *_*

Simply put:

If y'all are still targeting vertical tab selectors or specific elements, y'all are still doing it wrong.

I really... really... don't know how I can say this any clearer.

We are all contributing here. It would be nice if you could offer a constructive suggestion. You first told that data attributes should be used as selectors, and when pointed out that is not doable for verticaltabs you just state that you cant be clearer...

I guess we can either
- accept that we target vertical tabs items, which are replacable in the js theming and therefor require special attention when vertical-tabs.js is replaced
- or abandon vertical tabs entirely and use another form element to switch between crop styles.

@woprrr what is your opinion on swapping out vertical tabs for another UI?

@dkre I agree that it should work with every theme. This would mean that we need to abandon vertical tabs, since vertical tabs are created in vertical-tabs.js, which can be altered by any theme/module, as bootstrap does. (see #29)

The good part about vertical tabs is that it has decent theming for almost every theme, as it is a default widget. The bad part is, you can't target the tabs for javascript events and expect every theme which swaps out the vertical-tabs.js script to work with it too.

Personally I wouldnt mind switching to a dropdown solution similar to how manualcrop worked in d7.
@woprr: is this something you can relate to, or do you prefer keeping the vertical-tabs solution?

We are all contributing here. It would be nice if you could offer a constructive suggestion.

I have. Several times. I really shouldn't have to code something all the way out just for someone to actually understand the basic concept to get at least going.

I'm a bit confused about vertical tabs, why can't these have data selectors?

They can. They can be specified on the element that defines the #group (which, by default, is a <details> element). These are kept when converted into vertical tabs.

This would mean that we need to abandon vertical tabs

No it doesn't.

---

The real problem is with the entire JS file. The code is "architected" in so many anti-patterns that there's what I tend to call "selector dependency hell".

Everything is bound to very specific DOM traversal hierarchy: core's default output. This alone is what ties "vertical tabs" to the current JS and why other themes don't "work".

The reality is this: if the JS were to focus on just the elements it creates (by assigning the necessary data attributes to the elements it creates in PHP), it can implement the plugin it needs to without having to worry about vertical tabs at all.

That all being said, I suppose the only way anyone is going to understand what I mean is if I'm the one who does the work to abstract all this.

So here y'all go... works with any theme now and fixes quite a few issues...

Hi guys, Again thank you all to follow and discuss about these important issue !

I try to respond in correct order :

@woprrr what is your opinion on swapping out vertical tabs for another UI?

No, we doesn't change it, because these element be a serious change for the module during Zurich Media sprint 2015. IWC have too specific markup and use ul > li elements to display list and that indeed more complexity for Front devs. Then we have choose an more "standard" pattern and elements for drupal 8.

The opinion of @markcarver are correct ! We DO provide an architecture more abstract and more flexible to permit two big features :
1st : Respect JS patterns (but I admit that's not my speciality and I am counting on you to do that).
2nd : Permit to IWC users to changes the "structure" of elements and permit to customize (That is strongly linked to that issue that's a prerequire).

The real problem is with the entire JS file. The code is "architected" in so many anti-patterns that there's what I tend to call "selector dependency hell".

I admit the first "architecture" of these module have change too fastly during Zurich Media Sprint and I can't follow all changes, I delegate onto Front Devs but we did the better as possible.

That all being said, I suppose the only way anyone is going to understand what I mean is if I'm the one who does the work to abstract all this.

Yes ! That's the professionalism about Oncle Bob talk :), In all case I need to test/review/UNDERSTAND your vision NOW and take you a big THANK YOU :).

I try to call pivica to see that issue, because @markcarver have fixed these #2660786: Should we use throttling or debouncing for windows resize event? issue too O_O

patch #38 works very well with Bootstrap, if it should works for other themes we could consider this issue solved

First I re-roll patch to be applied more easily (js/ folder are placed on root of project not relatively of project, this is a small detail).

Thank a lot for these patch again, with small efforts we can solve theses problems and have a good results :) !

First Feedback

Simple cases :

1. When Edit/form error fired (hard/soft limit reached) the crop area are incorrect. (With all themes seven/boostrap/adminimal).
   Select your crop zone :
   
   All is okay on front comeback to edit it :
   Area have moved. The recovery of crop zone are altered.
2. When we use "Crop media example" with IEF to lie Media into Content. We are blocked the "add element" button not add and we are blocked in this state. In console log we have.

Uncaught TypeError: Cannot read property 'offsets' of undefined
      at HTMLDocument.<anonymous> (ckeditor.js?v=8.3.0-dev:298)
      at HTMLDocument.dispatch (jquery.min.js:3)
      at HTMLDocument.r.handle (jquery.min.js:3)
      at Object.trigger (jquery.min.js:4)
      at HTMLDocument.<anonymous> (jquery.min.js:4)
      at Function.each (jquery.min.js:2)
      at n.fn.init.each (jquery.min.js:2)
      at n.fn.init.trigger (jquery.min.js:4)
      at Object.displace (displace.js:81)
      at ToolbarVisualView.js:91

3. When we use "Crop File entity example" Add an file image click to "edit" to edit file in popin select crop area save file and save content. Anything happen, the element haven't be cropped.
4. When you use fields with cardinality unlimited we have two bugs. First When you select crop area add another element that loose your croped zone previously selected. Second select an specific element with your crop area. Save it the displayed croped image are not my previous selected area.

First I re-roll patch to be applied more easily (js/ folder are placed on root of project not relatively of project, this is a small detail).

I really don't understand what you're saying here. All patches should be based against the relative root directly of the project in question, per https://www.drupal.org/patch/create and https://www.drupal.org/patch/apply

Thank a lot for these patch again, with small efforts we can solve theses problems and have a good results :) !

You're welcome. Additional follow-up patches can be done by others as I don't have the time to continue working on this issue (I already maintain enough projects).

I only did this as a courtesy (over my spare time this weekend) to this issue/education to show that, yes, all this can be accomplished in an abstract way.

The "bugs" listed above are either existing issues (with the logic found in existing JS code) or intentional oversight due to not fully understanding what the "hard" and "soft" limits are intended to be and/or do (e.g. there really needs to be better documentation around what these are and what UI/UX implications it has).

I will gladly review any future interdiffs/patches on this issue to ensure it stays inline with the JS architecture I laid out.

I found a problem with the patch # 38, if I apply the patch to the latest version in development, loses the ability to limit the number of crop (selects all). I've described it here: https://www.drupal.org/node/2857252

Sorry, forgot to add the variable back when refactoring the unnecessary duplicate code.

Patch #44 is working fine

I flag this issue prior to solve during Media Sprints DDD Sevilla

I've having this same problem with my Bootstrap theme. I updated to 8.x-1.x-dev and then applied patch #44. However when I go to crop an image I get the following two exceptions:

Notice: Undefined index: #submit in Drupal\image\Plugin\Field\FieldWidget\ImageWidget::validateRequiredFields() (line 256 of core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php).

Warning: in_array() expects parameter 2 to be array, null given in Drupal\image\Plugin\Field\FieldWidget\ImageWidget::validateRequiredFields() (line 256 of core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php).

Any help is much appreciated, thanks!

*edited to remove unnecessary stack trace

@DarkstarTom, your problem is not related to #44 or this module, for that matter.

It's likely some other contrib module coupled with the following core issues:

#2745491: ImageWidget::validateRequiredFields() produces a PHP Warning message if triggering element is a non-button

#2715859: ImageWidget::validateRequiredFields() produces an error message if an image field is hidden with hook_entity_field_access()

A simple search would have have produced these issues.

Thank @markcarver to continue following these important issue. I had more time now to investigate have better following of these issue. IMHO I think we have one "critical" problem in editing mode. Actually I define a crop zone save all work well but in edit mode the cropped area aren't define at all. After investigation I am sure that provide to "Drupal.ImageWidgetCropType.prototype.initializeCropper" when that call "Drupal.ImageWidgetCropType.prototype.setValue". All value passed to js are incoherent and seems to be wrong. All calculations are based on $delta and that are equal to value. I admit not understand all mecanisms but the problem are initializedCropper with defaults value passed by element. I'm not really better to judge about this... I can only share my observations about problems.

Little screenshot on debug :

Just one little typo Fix to win a line in condition. I'm fully open to other fixes to finish this wonder patch :) The structure are really more simple/clear you right markcarver :)

Ok. I can take a look then. Just not entirely sure when, perhaps sometime this weekend.

Thank you a lot @markcarver :) In DDD Sevilla I had more time to continue with this issue.

Sorry this took a while. Had to upgrade OS over the weekend too.

Ok. This patch should make everything work. I flushed out the "values" a bit so they're more explicit.

I also went ahead and fixed the "summary" for the main wrapper. Unfortunately, core does not expose the summary attributes in any way, so these selectors have to be targeted manually (DOM traversal).

However, since these selectors are modifiable before anything is initialized, any contrib project can easily append/change it. An example of how this can be accomplished the proper way can be seen in the related issue I'm attaching now for Bootstrap.

Hi @markcarver,

Wow ! Wonderfull patch again :)

I'm actually focused to review it and test all cases possibles. At this point only one problem appear and this is for specific "tricky" cases. File entity integration are break (totally), I suspect a problem with changes not replicate in form_alter() or anythink of that. All other cases simple/responsive/Media/form APi integration work well and that is the more important at this point. I continue my tests if only File entity integration are break I think we can consider it GOOD and merge all changes and solve isolate break case appart.

I add one credit to capynet for help during DDD seville on this issue to understand and found solution to this subject :)

I would agree that any further issues should be created as separate follow-up issues. It'll be easier to track problems with smaller patches in dedicated issues i once this is in.

Setting issue to RTBC since it sounds like everything is good.

Just one thing ;) That fix Solve File entity/saves problems. The bug are simple in BuildToForm / BuildToEntity function we check if crop_element are applied == '1' after add/update crop area. In refactor you have avoid all duplicated/unneeded code and setValue at the same time but we doesn't change values['applied'] value we does have 0 or 1. This is a verry small adjust with that the patch is PERFECT to RTBC.

My test in ALL cases are complete :

Simple case : OK
Responsive case : OK
Media entity case : OK
File entity case : OK
Form API case : OK

Field cardinality illimited : OK (Works but sometime we can seen croped area little move without actions, we need to test it more/better after).
Mobile integration : OK
Resize windows (old) bug solved.
Mobile orientation change bug (old) solved.

Cool :D LGTM

+++ b/js/ImageWidgetCropType.js
@@ -619,7 +619,7 @@
+    if (delta && name != 'applied') {

Per the Drupal JavaScript Coding Standards, this should actually be using !==, but it's a very minor nitpick.

Oh my god !! You right !! It's small but that patch clean is verry important to follow your thought of clean JS/Pattern for that module and I'm the guarant of following that now !! Next important step :) Achieve a Tests to ensure we don't break any JS feature for the future changes. In DDD seville I found guys to goes in that direction :)

Merge it in dev branch + 2.x-rc branch immediatly

Thank a lot again for that really good job specially to markcarver for the solutions/works and for other to participate with me to test review changes in that issue. You make this module better !

Awesome :D Just glad to have helped. Hopefully this will show others how to build their module's JS in an abstract way so it can work in any theme.

Yes ! I have a lot of adventures to your side on this issue is fantastic. I owe you a bier now: P

PS : I think I'll be interested a little more in js this way ;)

I can confirm that today's dev release has fixed the problem and I now have Image Widget Crop working on my Bootstrap theme. Great work guys!

After another test (by chance) I have processed another complete re-install on all instance and Test dev branch (after merge on that issue).
That not appear on all tests previously because First I install fresh instance and second apply patch (kind of cache ?). But in this case the problem appear always.
We have a last bug describe onto #2862709: Crop first area are wrong when "Always expand crop area" is set to "YES" If you can seen that would be super ! I try to investigate more tomorrow about that.

@woprrr, I just realized that this was committed to 8.x-1.x in #61. It should be reverted as this is a major change and should only be applied to 8.x-2.x as you stated in #60.