Right now if have a form inside the preview, the values of that form are being collected. Also seems to have issue with randomly closing the pane config.

Needs work because there's some visual issues. Also needs to test further to see if it solves all the observed issues.

Historical note: This fixed #2111721: Pane auto-preview steals keyboard focus which was a super important bug!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hefox’s picture

Status: Needs work » Needs review
FileSize
5.61 KB

Moves it to a post_render, which seems to fix several bugs.

Right now the form is going through rebuild on autosubmit (see drupal_process_form and comment about multi step form), which means the preview is actually being created twice, which slows done the form build and can cause duplicate js settings issues (w/ fivestar module for example).

Some visual issues, but seems good enough to mark it as needs review.

hefox’s picture

Instead of refreshing the entire form, this updates it to just refresh the preview.

hefox’s picture

had a FALSE in the js

hefox’s picture

form rebuild is breaking the ajax cause of multi step form (grabs an stale form that doesn't have all the items, etc.), so try to disable that rebuild and just grab the new values for the preview.

hefox’s picture

values instead of input

hefox’s picture

Need to only cancel ctools autosubmit thing for ajax buttons. Wonder if there's a class to do this automatically mmm.

hefox’s picture

Andrew Edwards’s picture

Hi hefox,

I've applied your patch but I have ann issue with the live preview.

I have a basic text box. The first time I type in it the live preview updates and the save button switches to 'updating'. The second and subsequent times I type in it the preview does not update and the save button does not switch.

Let me know if you need any more info,

Andrew

hefox’s picture

Is it only updating the preview or updating the entire form?

Andrew Edwards’s picture

Status: Needs review » Closed (fixed)

Hmn.

I've done a clean install of panopoly and applied your patch. All seems to work well now so it must be something up with my previous install (I've lots of custom code).

I'll let you know if I figure it out.

hefox’s picture

Status: Closed (fixed) » Needs review
FileSize
7.51 KB

Don't close issues like that. The patch is still for review.

Fixing some weirdness; ctools ajax multistep weirdness.

The form that is gotten during one stage is incomplete, and then gets cached, so for now telling it not to cache that wrongly built form, but that really isn't the best solution.

hefox’s picture

I think I finally figured out the right magic to work with ctools multi step form

Since it doesn't go through normal drupal_get_form, instead ctools_content_form, need an ajax callback that uses that instead.

Everything seems to mostly just work once that is solved

populist’s picture

Status: Needs review » Needs work

I am seeing a couple issues related to new "fieldable panel panes". You can try to add a table widget with the patch to see.

1.) There is an argument error (Warning: Missing argument 7 for ctools_content_form) that comes from the ctools_content_form() call. I believe we need to add in the $plugin variable.

2.) There is an undefined variable (Notice: Undefined variable: preview_subtype in panopoly_magic_form_alter()) which I think is because $preview_subtype isn't always handled in extract($form['#panopoly_magic_preview_info']).

Otherwise, very cool improvement.

hefox’s picture

Status: Needs work » Needs review
FileSize
9.33 KB

addresses 1+2 and a bit of the tablefield (converts any #ajax without 'path' to use system/panopoly-magic) + #2050657: Using 'click' for the event handler of rebuild table prevents other ajax submit buttons from working addresses it further. Not sure where the .make file defines tablefield, none?

mpotter’s picture

Status: Needs review » Reviewed & tested by the community

Using this in Open Atrium 2.

mpotter’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
8.14 KB

Needs a reroll for RC5. Here is is. Since this was a complex patch, moving this back to Needs Review for testing.

mpotter’s picture

Status: Needs review » Needs work

This patch is causing an error when switching pane Styles.

1) Click the paintbrush Style icon for a pane.
2) Try switching to a different pane style.

You get this AJAX error:
PHP Fatal error: Unsupported operand types in /var/www/mysite/profiles/openatrium/modules/contrib/ctools/includes/content.inc on line 564

Called originally from:
ctools_content_form() /opt/development/mpotter/drupal/profiles/openatrium/modules/panopoly/panopoly_magic/panopoly_magic.module:64

mpotter’s picture

OK, worked with hefox on this (well, he did most of the work). Found that the Styles form does not use the ctools_wizard_multistep_form like the config settings does. So needed to alter the code in the ajax callback to handle this with drupal_build_form() instead.

Here is a new patch.

petr-oa’s picture

Hello! Thank you.

I am still not able change panel style and get this errors :

Browser message
An AJAX HTTP error occurred.
HTTP Result Code: 500
Debugging information follows.Path: /system/panopoly-magic
StatusText: Internal Server Error
ResponseText:

httpd log

PHP Fatal error: Unsupported operand types in /home/nocadmin/www/drupal-7.23/profiles/openatrium/modules/contrib/ctools/includes/content.inc on line 564

Installed
Drupal 7.23
and
openatrium-7.x-2.x-dev-no-core.tar.gz

httpd -v
Server version: Apache/2.2.3
php -v
PHP 5.4.19 (cli) (built: Aug 23 2013 12:25:29)

mpotter’s picture

petr-oa: be sure you also add the patch here #2016527: Panopoly Magic: use #parents instead of checking under exposed. Post here if you still have the problem after using both patches.

petr-oa’s picture

This patches included in OA2 distributive

[panopoly_magic]#pwd
/www/drupal-7.23/profiles/openatrium/modules/panopoly/panopoly_magic

[panopoly_magic]# cat PATCHES.txt
The following patches have been applied to this project:
- http://drupal.org/files/2016643_panopoly_magic_screw_pre_render_with_201...
- http://drupal.org/files/2017159_panopoly_magic_preview_post_render-16.patch

This file was automatically generated by Drush Make (http://drupal.org/project/drush).

mpotter’s picture

Status: Needs work » Needs review
FileSize
8.66 KB

OK, I think this is getting close. We found one more issue in Open Atrium 2 with this patch. It was causing a preview to display for the Region Styles page. The Preview shouldn't appear for Regions, just for Panes. So modified the patch in #18 to test for the $pane object. Here is the new patch. Let's get some more eyes on this!

dsnopek’s picture

Version: 7.x-1.x-dev » 7.x-1.0-rc5
Issue summary: View changes
Issue tags: +sprint

Adding the "sprint" tag - we're going to focus on this for the next release!

dsnopek’s picture

[Oops! Wrong issue. :-)]

dsnopek’s picture

Status: Needs review » Needs work

Ok, I've begun trying to review this! I'm using the following steps to test:

  1. Apply the patch
  2. Clear the Drupal cache (since menu callbacks are added, among other things)
  3. Make sure Panopoly Magic's "Live Preview" is set to "Automatic"
  4. Click "Customize this page" on some random IPE enabled page (the demo front page, usually)
  5. Add a "Table" widget
  6. Fill in the table fields and watch them appear in the preview

I first tried the patch in #22, and it just plain doesn't work for me. :-/ Nothing ever appears in the preview. I can Save, and it'll appear on the page correctly. When I click the "Settings" button again, it will be in the preview. But my changes are never automatically reflected in the preview.

Then I tried the patch in #18, and it will catch the first change I make and update the preview. But all subsequent changes won't cause the preview to update. Then I tried changing the "Live Preview" to "Manual" and clicking the "Update preview" button - this works! However, the "Style settings" page doesn't work, which it looks like what #22 was supposed to fix.

@mpotter: I know that @hefox isn't working on Open Atrium or Panopoly stuff right now - is there any chance you could take a look at this and put together a working patch?

Also, I don't have a steps to recreate the bug this is fixing. :-) It would be great to reproduce the failure and then apply this patch and see that it's working. @mpotter: Could you provide steps to reproduce the bug? Thanks!

For my part, I'm going to try and finish up the Behat test for live preview so that it'll be easier to test patches that affect it: #2150051: Add a Behat test for the live preview.

dsnopek’s picture

Attached is a new version of this patch that actually works in my manual testing. There's one bug I haven't been able to figure out yet - here are the steps to reproduce:

  1. Make sure the "Live preview" is set to Automatic
  2. Activate the IPE on a page that supports it
  3. Click the "Style" button
  4. Change the style to something else
  5. Click the "Next" button. Nothing happens....
  6. Click the "Next" button again. It saves!

Not sure why the next button has to be pressed twice if the preview has reloaded...

Now that this is working, I can confirm that it fixes #2111721: Pane auto-preview steals keyboard focus.

dsnopek’s picture

Status: Needs work » Needs review
dsnopek’s picture

It turns out that this code (added in this patch) was the culprit for making it necessary to press the "Next" button twice when on the style page:

     // Replaces click with mousedown for submit so both normal and ajax work.
     $('.ctools-auto-submit-click', context).click(function(event) {
       if ($(this).hasClass('ajax-processed')) {
         event.stopImmediatePropagation();
         $(this).trigger('mousedown');
         return false;
       }
     }

It also appears to break the triggerDisable function, because it's calling event.stopImmediatePropagation();.

Attached is a new version of the patch which removes this code. This still passes all my manual and automated tests - so, I'm not entirely sure what that code was supposed to be fixing.

@mpotter: Can you test this with Open Atrium and the use cases that this patch fixed for you to make sure it still works? Thanks!

mpotter’s picture

Will try to test this in the next couple of days.

It's been a while and hfox would probably have more information on this, so I'm trying to remember what this patch was fixing. If I recall, it was actually a bunch of different stuff related to how the Preview was being rendered and how that interfered with the Config popup.

The Preview appears both when using the Config Settings form for a panel, but it also renders for the overall preview when Adding new widgets to a page. When doing the Preview from the Add page, the context(s) needed to render a panel are often not available.

The Config form and the Style form use slightly different Ctools code to handle the popup. Some forms are Multipart (with the Next button) and some are not.

I believe this patch also addressed problem caused by the Preview update losing focus on various form elements. For example, in the past when you started typing in the Body field of a Text widget the update to the Preview would remove focus from the Body field that you were typing in.

Finally, in several Open Atrium widgets there is a Space and Section selection field and the Section field is recomputed whenever the Space is changed. I believe that before this patch the update of the Section field when changing the Space didn't work, or the Section field was reset somehow by the Preview updated.

Not sure if all of these old problems were fixed in this patch or in others. But in general we want to test as many different kinds of widgets (Multistep, non-multistep, Fieldable Panels Panes, OA2 panes with Space/Section, etc) and ensure they work properly.

hefox’s picture

If I recall correctly, the entire form is refreshed instead of just the preview on changing form values, so if you're typing into a text field like title and got autosubmit on, you get interrupted mid typing. If you're clicking checkboxes, only one click goes through at a time. This addresses this by only refreshing the preview instead of the entire form, using ajax, which isn't the easiest with ctools created forms

dsnopek’s picture

@mpotter / @hefox: Thanks! I look forward to any testing you can help out with. :-)

Unfortunately, I just noticed that my patch in #28 no longer fixes the focus issue! So, I'm going to have to put that code I removed back and find a new solution to the style issue. :-/ This is super delicate stuff - just when I thought I was done... Anyway, I'll hopefully have a new patch to post later today.

dsnopek’s picture

Ok, here is a new version of the patch which puts the code I removed in #28 back, BUT specifically excludes the "Style" page. I've got the feeling it might actually be better to include the one page we know this works for rather than exclude those we know it doesn't - but this is a good start.

Also, this bit of code breaks the triggerDisable(), but I can't come up with a way to implement that in conjunction with the new approach this patch takes, so I've removed it. And I actually don't think it's necessary any more, now that the preview is the only part being replaced, rather than the whole form.

Hopefully, this is the last version of this patch to come from me today! ;-) I await any testing that others find time to do!

mpotter’s picture

Status: Needs review » Reviewed & tested by the community

OK, did some testing of #32 in Open Atrium and it all seems good. Tried various complex widgets, both config forms and style forms.

I did have a case where I clicked the Next button twice on the Style form. I first changed the style and then I think I clicked Next while the Ajax callback was in progress and then had to click it again.

Wonder if the ajax spinner over next to the Save button needs to be a bit more prominent? Probably not something to worry about for this specific patch though.

I think this covers the error cases we were originally trying to fix.

dsnopek’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, Mike! Since this passes my testing, all our existing automated tests and works in Open Atrium - I've committed in 96d44be.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

dsnopek’s picture

Component: Code » Admin
Issue summary: View changes

Added a historical note to the issue summary, so I'd remember why we made such a drastic change!