Contact, register and comment forms do not prefill with user info from browser

Count me +1 for auto-loading form.js for all forms by default.

attached patch autoload misc/form.js into theming phase of each form. I inject JS into theme_form to add JS only for web request (drupal_get_form(...) can also be used to populate info from webservice, ...)

That needs to be [#attached][js] for the 'form' element type instead, defined in system_element_info(). See

http://api.drupal.org/api/function/system_element_info/7
http://api.drupal.org/api/function/drupal_process_attached/7

We also probably want to remove all or at least most manual loading of form.js throughout core. Watch out for the weight being used everywhere.

Re-rolled to use system_element_info. Also found all instances where we're manually loading misc/form.js and removed them since they're now covered by the attached js.

Thanks!

Good catches, committed to CVS HEAD. Thanks!

This patch seems to have broken the teaser splitter, the input format collapsing thing, and vertical tabs on the node/add form.

Re-tried from fresh install, same thing. Credits to merlinofchaos for tracking down the source of the problem.

Incidentally, the error from Firebug is:

this.summary.html(this.fieldset.drupalGetSummary()); is not a function in vertical-tabs.js line 132

<merlinofchaos> 10:39 webchick: My guess is that it changes the order of the javascript files

My guess is, he is right. :) Can someone take a look?

misc/form.js doesn't get included at all now.

(Edit: should have read the full issue before commenting u.u failfox).

I took a glance at the problem, and couldn't fix it - getting same error as webchick:

/d7-parish/misc/vertical-tabs.js?v=1.0:132
Uncaught TypeError: Object #<an Object> has no method 'drupalGetSummary'

$form += element_info('form');

the issue being that
$form['#attached'] is not empty before this is called for some form; node form has Array ( [css] => Array ( [0] => modules/field/theme/field.css ) ) .

I'm really not sure if this is the correct way to fix this though; moves it out of element_info and adds it in if hasn't been added in

I can confirm that hefox's patch in #12 fixes the issue for me...

Insure should be Ensure.

Why do we need to check if (empty) before adding it - if we always want to add it indiscriminately I don't think it matters if we overwrite it.

Ugh, we really need a good array_merge_recursive function that doesn't destroy on when arrays have the same keys.

Updating comment to mention why the empty check and Insure => Ensure; for the (likely very rar)e case a form wants to define it with a different weight.

Re-considered. We need to revert this, since we needlessly load jquery.js and form.js on all pages containing any form now. Which includes a simple search form that may be visible on all pages. Sorry for supporting the wrong direction.

1) Stray @todos in this patch.

2) c912374915 mentioned a cookie to form field name mismatch in the comment form that prevents the auto-population of form values. Not investigated for this patch.

I rolled back #4 to get us out of the "critical" level of this issue. Setting needs work for sun's todos in #17, and for fixing the original bug.

Re-rolled without @todos. Node and user admin listing pages need to be tested manually.

I'm not able to see the mentioned field name mismatch in the comment form. The cookie key is "homepage", the form input name is "homepage".

Thus, if aforementioned listing pages don't produce any JS errors, this patch should be good to go.

Someone needs to (manually) confirm that this patch works. This functionality cannot be verified by the testing framework yet.

.

Actually, that's what hook_library() is for.

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

Note that I'm perfectly willing to even set this to postponed until after 7.0 release, but since this is an actual non-API-changing bug report, I'm marking this back to 7.x.

then should this be #attached['library'] instead?

Hi, here a patch for D8 without cookie.

Will try to submit a D7 version soon.

Nice (re)start.

Since we can use funky array methods I'd refactor the code a bit to remove all the for(). Also update the way .once is used to be more in line with what we're doing in the rest of core.

      var $forms = $('form.user-info-from-browser').once('user-info-from-browser');
      if ($forms.length) {
        userInfo.map(function (info) {
          var $element = $forms.find('[name=' + info + ']');
          // same after.
        });
        
        $forms.on('submit', function () {
          userInfo.map(function () {
            // same as above  
          });
        });
      }

Updated patch.

Fixing coding standard for variable name and try to not modify field with data.

I'm not really happy with the explicit usage of 'name'. Maybe I can fill an attribute with the default value and test that instead ?

<input type="text" id="edit-name" name="name" data-default-value ="Anonymous" value="Anonymous" size="30" maxlength="60" class="form-text">

Here is the D7 patch. I started / rerolled from the sun #22 patch.

D8 version with no explicit usage of specific field name.

For readability sake can you put ($element.val() === '' || ($element.attr('data-drupal-default-value') === $element.val())) in a variable and use the variable in the if?

Also replacing the class .user-info-from-browser by a data attribute to select in JS? Otherwise Butch will not be happy with you :p

Making Butch happy

Making HTML5 happy now

And making nod_ happy !

All good now everyone is happy and work as expected for comments, new account and contact page :)

testbot

This looks good but I think we need to change record to cover the changes.

Created change record draft: https://drupal.org/node/2243627

Patch still works and applies. Change record is ok.

Alexpott asked to add before-after examples in the change record and I did that. After another review, found the next issue:

+++ b/core/modules/user/lib/Drupal/user/RegisterFormController.php
@@ -44,8 +44,7 @@ public function form(array $form, array &$form_state) {
-    $form['#attached']['library'][] = 'core/jquery.cookie';
-    $form['#attributes']['class'][] = 'user-info-from-cookie';
+    $form['#attributes']['data-user-info-from-browser'] = TRUE;

I guess it lacks here the
$form['#attached']['library'][] = 'core/drupal.form';?

I want to review that the register form actually works, will get back to it later today.

I didn't found where it is added, but it is definitely there and works, so back to RTBC.

Retitled for mentioning concrete forms instead of modules, and for clarifying that it is in the browser, not a cookie anymore.

Thank you penyaskito for the change record. I would like to add some clarification, but not sure if I have to edit the change record or not, so I add it here.

It's about how to use it in our form. Currently it works only for the following named input 'name', 'mail', 'homepage'

Of course you can edit it. Any clarifications are welcome :)
Please go ahead, and thanks for checking it!

36: 749748-36-d8.patch queued for re-testing.

Testbot was failling, tests are ok now

Straight reroll.

+++ b/core/modules/comment/lib/Drupal/comment/CommentForm.php
@@ -167,6 +167,9 @@ public function form(array $form, array &$form_state) {
+ $form['author']['name']['#attributes']['data-drupal-default-value'] = \Drupal::config('user.settings')->get('anonymous');

Can we inject this?

Also there's a call to user_cookie_save() in MessageForm::save() that can be removed here I believe, using local storage instead.

Rerolled manually, no interdiff provided, sorry.

@tim-e: Changed that. No need to inject, it is already injected in HEAD.

@larowlan: Removed user_cookie_save().

Auto-merged, didn't apply.

+1 RTBC - over to nod_ for one last look at JavaScript

This works as expected on contact page.

The comment page has an other urelated issue:

    if (!$this->currentUser->isAuthenticated() && $anonymous_contact != COMMENT_ANONYMOUS_MAYNOT_CONTACT) {
      $form['#attached']['library'][] = 'core/drupal.form';
      $form['#attributes']['data-user-info-from-browser'] = TRUE;
    }

This is never true. Changes to comment settings on the content type are not being saved properly.

When the code enters the if, everything works as expected. I'm putting this RTBC and a followup will be needed for comment field configuration saving (if it's not my setup the problem).

Committed 5afbef9 and pushed to 8.0.x. Thanks!

@nod_ looks like we might not have test coverage...

      // @todo Complete test coverage for:
      //'comments'        => array(CommentItemInterface::OPEN, CommentItemInterface::CLOSED, CommentInterface::_HIDDEN),
      //// COMMENT_ANONYMOUS_MUST_CONTACT is irrelevant for this test.
      //'contact '        => array(COMMENT_ANONYMOUS_MAY_CONTACT, COMMENT_ANONYMOUS_MAYNOT_CONTACT),

Opened #2318251: Make comment links functionality testable and convert CommentLinkTest to PHPUnit for #61, #62

The change record for this is still in draft state.

(See #42 and #48.)

I've already add my remark from #48 in the change record, i don't know what i need to do now. Should I publish the change record myself or maybe penyaskito or nod_ should do it ?

I published the change notice.