Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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!
Comments
Comment #1
hefox CreditAttribution: hefox commentedMoves 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.
Comment #2
hefox CreditAttribution: hefox commentedInstead of refreshing the entire form, this updates it to just refresh the preview.
Comment #3
hefox CreditAttribution: hefox commentedhad a FALSE in the js
Comment #4
hefox CreditAttribution: hefox commentedform 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.
Comment #5
hefox CreditAttribution: hefox commentedvalues instead of input
Comment #6
hefox CreditAttribution: hefox commentedNeed to only cancel ctools autosubmit thing for ajax buttons. Wonder if there's a class to do this automatically mmm.
Comment #7
hefox CreditAttribution: hefox commentedtypo
Comment #8
Andrew Edwards CreditAttribution: Andrew Edwards commentedHi 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
Comment #9
hefox CreditAttribution: hefox commentedIs it only updating the preview or updating the entire form?
Comment #10
Andrew Edwards CreditAttribution: Andrew Edwards commentedHmn.
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.
Comment #11
hefox CreditAttribution: hefox commentedDon'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.
Comment #12
hefox CreditAttribution: hefox commentedI 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
Comment #13
populist CreditAttribution: populist commentedI 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.
Comment #14
hefox CreditAttribution: hefox commentedaddresses 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?
Comment #15
mpotter CreditAttribution: mpotter commentedUsing this in Open Atrium 2.
Comment #16
mpotter CreditAttribution: mpotter commentedNeeds a reroll for RC5. Here is is. Since this was a complex patch, moving this back to Needs Review for testing.
Comment #17
mpotter CreditAttribution: mpotter commentedThis 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
Comment #18
mpotter CreditAttribution: mpotter commentedOK, 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.
Comment #19
petr-oa CreditAttribution: petr-oa commentedHello! 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)
Comment #20
mpotter CreditAttribution: mpotter commentedpetr-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.
Comment #21
petr-oa CreditAttribution: petr-oa commentedThis 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).
Comment #22
mpotter CreditAttribution: mpotter commentedOK, 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!
Comment #23
dsnopekAdding the "sprint" tag - we're going to focus on this for the next release!
Comment #24
dsnopek[Oops! Wrong issue. :-)]
Comment #25
dsnopekOk, I've begun trying to review this! I'm using the following steps to test:
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.
Comment #26
dsnopekAttached 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:
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.
Comment #27
dsnopekComment #28
dsnopekIt 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:
It also appears to break the
triggerDisable
function, because it's callingevent.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!
Comment #29
mpotter CreditAttribution: mpotter commentedWill 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.
Comment #30
hefox CreditAttribution: hefox commentedIf 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
Comment #31
dsnopek@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.
Comment #32
dsnopekOk, 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!
Comment #33
mpotter CreditAttribution: mpotter commentedOK, 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.
Comment #34
dsnopekThanks, Mike! Since this passes my testing, all our existing automated tests and works in Open Atrium - I've committed in 96d44be.
Comment #36
dsnopekAdded a historical note to the issue summary, so I'd remember why we made such a drastic change!