\Drupal\Core\Render\Element\Button::preRenderButton allows to explicitly remove .form-submit. Markup purists could then theoretically do it.

For various reasons (e.g. #2831459: page_load_progress doesn't respect form validation) we want to either only support .form-submit or add a new class we'll know is going to be there always.

CommentFileSizeAuthor
#12 interdiff-6-12.txt517 bytesanavarre
#12 2883036-12.patch8.25 KBanavarre
#7 2883036-6.patch8.11 KBanavarre
#6 interdiff-5-6.txt1.3 KBanavarre
#5 interdiff-4-5.txt1.14 KBanavarre
#5 2883036-5.patch7.73 KBanavarre
#4 2883036-4.patch7.54 KBanavarre
#2 2883036.patch9.5 KBanavarre
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

anavarre created an issue. See original summary.

anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented
Status: Active » Needs review
FileSize
2883036.patch9.5 KB

In case we decide to go with .form-submit, here's a PoC that looks to be working okay.

Dom.’s picture

Dom. CreditAttribution: Dom. as a volunteer and at ACINO commented

I understand that markup purist *could* remove submit-form class, ending up with a not anymore working module. Those *power users* need some skills to do so, and would have thus also the skill to create a custom submodule (or theme) running a specific hook. For this reason and to keep some flexibility to power users, I would suggest the following:

  1. +++ b/config/install/page_load_progress.settings.yml
@@ -1,5 +1,4 @@
-page_load_progress_elements: '.form-submit'

    Keep a configuration this kind. 

  2. +++ b/js/page_load_progress.js
@@ -13,19 +13,17 @@
+      var form_class = String('.form-submit');

    Do not hardcode here, but keep using the configuration. 

  3. +++ b/js/page_load_progress.js
@@ -13,19 +13,17 @@
-      for (var i in exit_elements) {

    Keep using the loop over the configuration. 

  4. +++ b/page_load_progress.module
@@ -64,7 +64,6 @@ function page_load_progress_page_attachments(array &$attachments) {
-      'elements' => $config->get('page_load_progress_elements'),

    Keep this thus ! 

  5. +++ b/src/Form/SettingsForm.php
@@ -45,14 +45,6 @@ class SettingsForm extends ConfigFormBase {
-    $form['page_load_progress_elements'] = [

    Indeed, do not let configuration this kind via admin UI.
    Instead, add up a configuration to exclude some form via there form_id (similarly to IFE module). Potentially using wildcarded form IDs.

NOTES:

  1. The default configuration could be a class we add on our own (not .form-submit but .throbber-submit for instance). We would control the adding of this class via a preprocess, but excluding AJAXED enabled class for instance (unless using submit() method rather then .click() actually solve issues like 'view' or 'inline entity form' modules compatibility).
  2. Let users exclude some form via the admin UI using there form_id (similarly to IFE module).
  3. Throw the classes to the JS via configuration (just as done now), but run this through a hook before "page_progress_throbber_classes_alter(&$classes) for instance. This would keep flexibility for power users.

Hopefully my english is clear enough so theses explanations can be understandable :)

anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented
Priority: Normal » Major
FileSize
2883036-4.patch7.54 KB

Attached patch is the result of a coding session / brainstorming with Dom. - No interdiff since this is a completely different approach. The idea here is we're trying to be as flexible as can be by adding a new CSS class .page-load-progress-submit which allows us to not rely on core's .form-submit anymore. The critical point here is we're no longer triggering a throbber for Ajax-based forms. This will help with fixing issues such as #2823968: Entity browser issue.

Another issue that should be fixed now is HTML 5 validation that was causing troubles in #2831459: page_load_progress doesn't respect form validation. This has been made possible by using .parents('form').submit(function () ... ); in the JS file.

Unrelated to this issue, we've also made sure to bring consistency to the module by removing any occurrence to 'spinner' and use 'throbber' instead. In the same vein, we've dropped using the confusing JS variable name exit_elements and have settled on elements instead.

anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented
FileSize
2883036-5.patch7.73 KB
interdiff-4-5.txt1.14 KB
anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented
FileSize
interdiff-5-6.txt1.3 KB

Now with an additional test.

anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented
FileSize
2883036-6.patch8.11 KB
Dom.’s picture

Dom. CreditAttribution: Dom. as a volunteer and at ACINO commented

Apart for the following glitch, seems fine to me as long as this seems functionally ok too (just read through the code, did not actually applied the patch).

+++ b/config/schema/page_load_progress.schema.yml
@@ -16,4 +16,4 @@ page_load_progress.settings:
-      label: 'Allow ESC key to kill the throbber'

Why remove this line ?

anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented 
+++ b/config/schema/page_load_progress.schema.yml
@@ -16,4 +16,4 @@ page_load_progress.settings:
-      label: 'Allow ESC key to kill the throbber'
\ No newline at end of file
+      label: 'Allow ESC key to kill the throbber'

It's not actually being removed.

If you could try and apply the patch and click around that'd be great so that you can then RTBC. On my end it works perfectly and I closed the HTML 5 validation / Entity browser issues because this does solve both problems.

Dom.’s picture

Dom. CreditAttribution: Dom. as a volunteer and at ACINO commented

Sorry did not see it ! Indeed

Dom.’s picture

Dom. CreditAttribution: Dom. as a volunteer and at ACINO commented
Status: Needs review » Reviewed & tested by the community

When applying:
2883036-6.patch:92: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

Beside this ok: RTBC

anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented
FileSize
2883036-12.patch8.25 KB
interdiff-6-12.txt517 bytes

Okay. This should apply better.

anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented

Credited Dom.

  • anavarre committed 3e2323a on 8.x-1.x authored by Dom. 
    Issue #2883036 by anavarre, Dom.: Explore supporting only .form-submit...
anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented
Status: Reviewed & tested by the community » Fixed

Fixed, thanks!

Status: Fixed » Closed (fixed)

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

jomarocas’s picture

jomarocas CreditAttribution: jomarocas commented

packport for drupal 7?

anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre at Acquia commented

Sorry, no. The D7 branch is bug fix only.