Ensure sticky action buttons to work with modals and ajax refresh-calls

@jurgenhaas, I've applied the MR !479 as a patch and it applied with no errors, After applying the patch, the action buttons are being rendered when Enable sticky action buttons theme setting option is enabled. I have attached the before and after fixes video clips for reference. Since the MR resolves the issue moving the issue status to RTBC.

+1 working splendidly for me as well, thanks for this patch.

saschaeggi → changed the visibility of the branch 3463796-ensure-sticky-action to hidden.

saschaeggi → changed the visibility of the branch 3463796-ensure-sticky-action to active.

Unfortunately after intensive testing I discovered some issues. The easiest way to reproduce this is when you edit a field in an entity type, e.g. https://gin.lndo.site/admin/structure/types/manage/article/fields/node.a...

and change the Allowed number of values

This triggers an Ajax request and breaks the Save button. Additionally once the Ajax request gets executed the form actions get appended to the end of the form.

Just a thought: As the disallow list seems to grow every day I think the best solution might actually be to use an allowlist and an opt-in system. This way we can control better which forms we're altering.

As I don't see a solid solution in Drupal core to check if an Ajax request happens in a modal/dialog/offcanvas.

I'll have a look later today. I would hope that we should be able to recognise if we are in a form rebuild, which should be handled differently, i.e. not handling any action button. I'll get back when I know more.

I would hope that we should be able to recognise if we are in a form rebuild, which should be handled differently, i.e. not handling any action button.

Agreed, but the buttons get re-rendered in an Ajax call in Drupal, as Drupal alters the form and form elements ids with adding a unique hash and this also includes the form actions 👀

I've rebased the MR first, that's done.

Agreed, but the buttons get re-rendered in an Ajax call in Drupal, as Drupal alters the form and form elements ids with adding a unique hash and this also includes the form actions

That's correct, but we could probably handle that. I've just tried manually and that seems to work:

• Ignore action buttons completely in that context so that the updated form will be sent without the action buttons that received special handling originally. That keeps those in place where they had correctly been placed originally.
• Hook into the ajax response and add an update command to search and replace the form attribute in action buttons to replace the old form ID with the new one.

@saschaeggi do you have an idea where and how we could achieve step 1? I could then look into the second one.

Versión de Drupal
10.3.2
Gin 8.x-3.0-rc13

When I activate:

Enable sticky action buttons
Displays all actions of the form in the sticky header.

There are no save or cancel or dellete buttons on the views popup configurations.

This was fixed, check out the latest dev release.

@jurgenhaas

@saschaeggi do you have an idea where and how we could achieve step 1? I could then look into the second one.

I don't think that's easily solvable.

Can you push your changes? We got some more issues in, and I lean towards switching to a opt-in mode.

I lean towards switching to a opt-in mode

While that would certainly be the easiest way forward, I'm concerned about both the DX and UX. Most developers won't opt-in and the result will be a terrible mixture for the end-user. The finish line seems to be so close, I'd like to give this another try. I'm a bit busy the next couple of days, but I'll give this another go right afterwards, if that's OK with you.

While that would certainly be the easiest way forward, I'm concerned about both the DX and UX. Most developers won't opt-in and the result will be a terrible mixture for the end-user. The finish line seems to be so close, I'd like to give this another try. I'm a bit busy the next couple of days, but I'll give this another go right afterwards, if that's OK with you.

Completely agree here. Let's collaborate here. Just want to make sure we either find a solid solution or we may continue with opt-in and postpone all forms for after a stable release 🤝💙

I've made a couple of clean-ups to the GinContentFormHelper class and can report that everything seems to be working without a block or allow list. Here is my test series:

• Content and non-content forms
• Settings form with included ajax requests, e.g. changing field settings by increasing the cardinality
• Modal forms like e.g. in views
• 🐛 Problem since rc11 with Action buttons Active : it now works, just that the "Next" action button is in the hamburger menu, but that should be solved in multi_setp by adding #gin_action_item = FALSE to their action button
• 🐛 The save button is missing for the media directories forms. Active : it works for the reported media directories form. There is a remaining issue with their media directories add form because that builds a popup over the modal which also contains a form, but that's been done in an ajax wrapper instead of a drupal_modal wrapper. If that were corrected, even that one would work out of the box. I'm leaving a comment for that in the other issue.

What's essentially changed is that Gin action button processing is disabled for drupal_modal and drupal_dialog only, but it still works on simple ajax calls. That results in updated form IDs for rebuilds to also update the sticky action buttons, which is really nice.

So, as far as I can tell, things are working really well. Next steps should be:

• More tests by other people.
• Clarifying if media_directories could change their second level modal into a drupal_modul instead of an ajax wrapper
• Going back to Gin's code and verifying if any of the exception handlings could now be removed

Just found 1 popup in views, which doesn't work yet. While apparently all popups in the views UI work as expected, there is one exception to that: re-arranging fields/filters/relationships/etc. - those are missing the action buttons. That looks like a similar issue to what we see with the second popup in media directories. Maybe that helps to identify a pattern to even address that in a generic way. I'll have another look.

It was the views_ui_rearrange_ form ID that needed to be added to \Drupal\gin\GinContentFormHelper::stickyActionButtons. Back to NR.

I've tested the new approach with all the cases in #18. Most use cases look really good.

Just found 2 issues:

• When going through the multi-step form, I'm seeing warnings like these: Warning: Undefined array key "#type" in gin_form_after_build() (line 65 of themes/contrib/gin/includes/form.theme).
• When using the media directories module, the main dialog works, but adding new media is showing with action buttons.

Other than those edge-cases, I haven't found any other issues. Well done @saschaeggi !!!

saschaeggi → changed the visibility of the branch 3463796-ensure-sticky-action to hidden.

saschaeggi → changed the visibility of the branch 3463796-ensure-sticky-action to active.

Tested this issue woking well for me attached media for Gin and Olivero Theme.

Hi!, I've tested the MR !479 and it's resolving most of the issue for sticky action buttons issues. After applying the MR as a patch the sticky action buttons for media_directory, views, simple_multistep, content forms. Below are my observations:

• Sticky actions button is working fine on field settings by increasing the cardinality: Allowed number of values configurations
• Now the action buttons are also being rendered in the views UI forms like field rearrange, relationships, contextual filters.
• While using the simple_multistep module the actions button is now being shown. However, I'm also encountering the same issue as mentioned by @jurgenhaas in #22 🐛 Ensure sticky action buttons to work with modals and ajax refresh-calls Needs review . Although the save button is present in the form but the Save button is not working as expected. Getting the error messages Warning: Undefined array key "#type" in gin_form_after_build() (line 65 of themes/custom/gin/includes/form.theme).
• When using the media_directories module the Save and select button is now being displayed and working fine. I'm not getting any issue while adding any media.

In addition, I've noticed that after applying the MR as a patch gin-sticky-form-actions class div is being added in DOM twice even though only single Sticky actions button is appearing as shown in the attached screenshots. There are two different id for the sticky actions button one with id #edit-gin-sticky-actions and other with #edit-actions. Before patch there was only single gin-sticky-form-actions div with two .form-actions class was there in the DOM. Not sure about the presence of the other gin-sticky-form-actions. However, this is not impacting any functionality and getting no issues while using the theme.

I've attached screenshots of before and after fixes for your reference.

Thanks!

When going through the multi-step form, I'm seeing warnings like these: Warning: Undefined array key "#type" in gin_form_after_build() (line 65 of themes/contrib/gin/includes/form.theme).

While using the simple_multistep module the actions button is now being shown. However, I'm also encountering the same issue as mentioned by @jurgenhaas in #22. Although the save button is present in the form but the Save button is not working as expected. Getting the error messages Warning: Undefined array key "#type" in gin_form_after_build() (line 65 of themes/custom/gin/includes/form.theme).

@Jürgen @tirupati_singh I've added a check for type, can you please have another look if it's fixed now?

Ok, I'll look into it again.

Hi @saschaeggi, I've tested the new changes made and after the new changes the actions button is not working for simple_multistep module. The Next button is not working for simple_multistep module, no action is being triggered on clicking the next button. Additionally, the actions button is also not being shown for node-edit form either when the Enable sticky action buttons is enabled or disabled. The recent changes has also affect other form-edit also, on enabling the Enable sticky action buttons the Save button is also not being shown on the theme setting page. Attaching the before and after screenshots for your reference.

@tirupati_singh there was a typo I'm afraid, I've pushed a fix. If you mind having another look if it works now 🙇

I'm having a memory exhaustion problem with this patch together with the simple_sitemap → module, but I'm not sure on how to solve it...

Fatal error: Allowed memory size of 1073741824 bytes exhausted (tried to allocate 20480 bytes) in /var/www/html/web/core/lib/Drupal/Core/Render/Element/RenderElementBase.php on line 177

Looks like it's related to this change

-        $form['status']['#group'] = 'gin_actions';
+        $form['status']['#group'] = 'status';

Maybe bc simple_sitemap uses the $form['status'] for other stuff other then the publish option (ref)? Idk

It could "solve" the problem for by just updating the 'status name to smth else

 - $form['gin_sticky_actions']['status']
 + $form['gin_sticky_actions']['status2']

but I don't think that would be the best idea...

Idk if that's smth Gin should worry about, or smth to change on the sitemap_module, but I have no idea if there are other modules also using the $form['status'] for smth else, that could cause problems with this implementation... If you think that's not a problem for Gin, feel free to update the status back to Needs Review.

@saschaeggi, okay I'll review it.

@saschaeggi, I've reviewed the latest changes for the typo fixes. Now the sticky actions button issue has been resolved successfully for node edit, views as well as for simple_multistep. The actions button is working fine when using the simple_multistep module and the node is now being save.

However, I'm also encountering the same issue Fatal error: Allowed memory size of 1073741824 bytes exhausted (tried to allocate 20480 bytes) in /app/web/core/lib/Drupal/Core/Theme/ThemeManager.php on line 204 as mentioned by @hmendes in the comment #32 🐛 Ensure sticky action buttons to work with modals and ajax refresh-calls Needs review for simple_sitemap module while using the gin theme. The issue has aroused after the typo fixes. Hence, I'm keeping the status to Needs work although the issues for node edit, views, simple_multistep has been resolved.

I'm attaching the screenshots of before and after fixes for your reference.

Thank you @hmendes for your analysis, that helped a lot to find a solution for this. I've just committed a change that will only set the #group to "status" if the status field exists and already has a group value. This is the case for all the cases I found and resolves the issue with simple_sitemap who also use "status" for a field which is completely different from all other usages.

I'm going to suggest to simple_sitemap that they rename that field at their end. It has no other relevance to them anyway.

However, that leaves another side effect that I haven't noticed elsewhere:

We get the empty gin more-actions trigger above the fieldset, even if we rename their status field and leave Gin unchanged. @saschaeggi any idea how that could happen?

Other than that, maybe worth another review now.

Hi @jurgenhaas, thanks for the quick fixes. I've reviewed the latest changes made for simple_sitemap and confirm that the sticky actions button is working fine while using the module on gin theme. The memory allocation issue has been resolved successfully. And the sticky actions button is working fine for node edit, views as well as simple_multistep module.

However, I didn't encounter the issue mentioned in the comment #35 🐛 Ensure sticky action buttons to work with modals and ajax refresh-calls Needs review . While using the simple_sitemap module, I'm not getting any gin more-trigger action buttons (three dots) on gin theme. I'm attaching the screenshots of before and after fixes for your reference.

As the mentioned issues have been resolved successfully and didn't get any issues while using the theme hence, moving the issue status to RTBC.

Thanks!

@Jürgen @tirupati_singh are we ready to merge here or is another round of testing needed?

@saschaeggi I've tested heavily in development and production the last few days and I did not come across any further issue. So, I'd be ok with merging this.

Awesome, thanks Jürgen for your hard work on this. Let's merge this so we can get a new release out this week 🎉

@saschaeggi, I've re-reviewed the recent changes. Everything is working fine and the MR can be merged.

Thanks all for working & testing this 🙇

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