Adobe-Consulting-Services / acs-aem-commons

http://adobe-consulting-services.github.io/acs-aem-commons/
Apache License 2.0
453 stars 600 forks source link

Parsys Limiter does not work with cq.authoring.editor.hook #1830

Closed reggie7 closed 5 years ago

reggie7 commented 5 years ago

Expected Behavior

Parsys Limiter's How To Use says

Add a wrapper client library which embeds category acs-commons.cq-authoring.add-ons.touchui-limit-parsys to your project with category cq.authoring.editor.hook. That category gets automatically loaded in the Touch UI’s editor frame.

One could expect it to work just like that then.

Actual Behavior

Parsys Limiter does not work in 6.3 by just following the How To Use explanation. It does, though, work after changing cq.authoring.editor.hook into cq.authoring.editor.

Steps to Reproduce

  1. Open a clean vanilla AEM 6.3,
  2. Optionally - install sp/cfp,
  3. Install ACS AEM Commons 4.0.0,
  4. Follow How To Use,
  5. Result: the feature is not available.
  6. Change cq.authoring.editor.hook to cq.authoring.editor,
  7. Result: the feature is available.

Links

I've created a quick sample usage branch with the following diff that illustrates the problem (I believe I've followed the instructions properly). Upon installing the project the page localhost:4502/editor.html/content/mobirise/enigmatic/en/blog.html should be checked for the feature and the above described workaround. After the workaround:

1  this should pop up on cq authoring editor 2  cq authoring editor makes the limiter work

reggie7 commented 5 years ago

I think that the feature does not work as expected in any combination:

First of all - the steps in How To Use, though being very simple, do not always suffice (cq.authoring.editor.hook vs cq.authoring.editor). Some combinations resulted in an error (different from the screenshot above) whenever I was trying to add or remove a paragraph. Some combinations load the js clientlibs (the above alert shows up), but do not pick the limit enforced.

kwin commented 5 years ago

@reggie7 Thanks for the report. Looking at the source code (for AEM 6.5) to me it seems that the Touch UI Editor (/libs/cq/gui/components/authoring/editors/core/body.jsp) loads the clientlib category cq.authoring.editor.sites.page which embeds cq.authoring.editor.hook among others. I will check more closely though.... Adobe recommends though to leverage the category cq.authoring.editor.sites.page.hook (for new extensions, compare with https://helpx.adobe.com/experience-manager/6-4/sites/developing/using/customizing-page-authoring-touch.html).

Update: I just added a simple clientlib with category "cq.authoring.editor.hook" and it definitely gets loaded in AEM 6.5. Have you used some overlays in your system?

reggie7 commented 5 years ago

@kwin thanks for your response. Might be then that it's just a matter of updating the docs. Let me try cq.authoring.editor.sites.page.hook though as well soon instead.

davidjgonzalez commented 5 years ago

@reggie7 does this appear to be a documentation issue? do you know when the new client library category (cq.authoring.editor.sites.page.hook) was/is recommended? we support 6.3+ so our docs should reflect supporting the various aem versions.

reggie7 commented 5 years ago

@davidjgonzalez - well, it depends. If it was supposed to work with cq.authoring.editor.hook as per the docs - it's a Commons package issue. If it's supposed to work with cq.authoring.editor - it's an issue with the docs. Regarding cq.authoring.editor.sites.page.hook - I don't know, maybe @kwin can help?

reggie7 commented 5 years ago

@kwin - The cq.authoring.editor.hook solution gets loaded in 6.5 actually. The alert is shown. But later the Parsys Limiter does not work anyway. It does both - get loaded and work with cq.authoring.editor. Sorry for having forgotten to mention it earlier. cq.authoring.editor.sites.page.hook - does not change the outcome unfortunately for me.

So to be as clear as possible, let me rephrase the problem in a single sentence:

The feature does not work, when following the docs to the word.

The goal would be then to simply have the feature work and the docs reflect what needs to be done to achieve this.

My remark regarding cq.authoring.editor is merely an additional observation aimed in helping out possibly.

kwin commented 5 years ago

Thanks for the additional hint. Looks like some ordering issue then, i.e. some dependency not yet loaded. I will investigate.

reggie7 commented 5 years ago

@kwin - any news here? Otherwise I'll be investigating more deeply myself soon.

kwin commented 5 years ago

Sorry, didn't have any chance to investigate more. So any help would be highly appreciated here.

reggie7 commented 5 years ago

@kwin (maybe @davidjgonzalez too?) - I'm pretty sure it's all caused by usage of embed. It's not transitive somehow. Should it be transitive in theory? All works just fine with dependencies which are transitive in practice.

So in addition to above configurations I've run the following code on 6.3, 6.4, 6.5: categories="[cq.authoring.editor.hook]" dependencies="[cq.authoring.editor.sites.page,acs-commons.cq-authoring.add-ons.touchui-limit-parsys]" and it worked like a charm. I've also tried categories="[cq.authoring.editor.sites.page]" with dependencies on 6.5 and got a positive result as well. cq.authoring.editor works with embed most likely because there is no transitivity needed then.

Since all of it gets complex to explain regarding all possible configurations, here's my summary of what would be the solution anyway:

  1. Let's use dependencies in the docs instead of embed,
  2. Let's establish whether embed should be transitive and if so - issue a daycare ticket.

What do you guys think?

reggie7 commented 5 years ago

Proposed docs fix: 164.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.