Problem/Motivation

On current HEAD, when you enter a label into a text field that is the 'source' for an element with #type => 'machine_name', it registers the key presses later slowly, if the label contains a space, one of the following characters (!@#$%^&*), or an international character (like ąóęńćśżźł).

Allowed after -beta?

The priority is set to "Normal" based on Priority levels of issues because it doesn't render the whole system unusabled and doesn't have "significant repercussions" (it really only affects this one Form API type).

Fixing this regression in UX behavior is a prioritized change., because it's usability and user experience improvement.

The impact is greater than the disruption: the impact is that it fixes the usability bug, and there is no disruption because this is limited to the event callback for this in JavaScript.

Steps to reproduce

  • Install latest HEAD of Drupal 8, using the standard profile
  • Go to: /admin/structure/types/manage/article/fields
  • In the "Add new field" text field, enter: Here is a field label
  • Press the TAB key and wait
  • You'll see the machine name slowly type itself out. After about 20-30 seconds it'll be done.

The key is to put spaces in the label! If you don't include spaces or other special or international characters (like described above) then it won't happen.

Here is a video of the behavior and a screenshot:

Lots of jquery ajax requests in firebug

Proposed resolution

  1. Wait until the user hasn't been typing for 300 milliseconds before making the AJAX request
  2. If the user types before the AJAX request has returned, abort it and start a new one (rather than starting so many AJAX requests at once)

Remaining tasks

  • Write patch
  • Manual testing done in #23, #19, and #17
  • Code review on patch
    • Is this an acceptable solution even if it is a change to Drupal 7 behavior? @dsnopek responds to this in comment #21
  • Commit
  • Profit!

User interface changes

The machine name will now update after the user has stopped typing, rather than as they are typing (although, as shown in the video, that didn't really work anyway).

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

clemens.tolboom’s picture

Status: Active » Postponed (maintainer needs more info)

I saw this behavior last Thursday but currently on Firefox cannot reproduce. It never took 30 seconds but I've seen waiting for the last letter 10 secs. IIRC it never was bad on Chrome.

Set to "needs more info" to let @dsnopek retest.

Please report with a browser matrix if possible.

dsnopek’s picture

I tested only on Chrome 36.0.1985.143 on Linux. I could have tested with Firefox, because I have that too, but didn't. I'll give it a try at some point later...

clemens.tolboom’s picture

I just learned this is due to callback to Drupal

    transliterate: function (source, settings) {
      return $.get(drupalSettings.path.basePath + 'machine_name/transliterate', {
        text: source,
        langcode: drupalSettings.langcode,
        replace_pattern: settings.replace_pattern,
        replace: settings.replace,
        lowercase: true
      });
    }

so watch your network interactions too. It explains the weird behaviour.

We can fix this for space in by having it replace spaces on baseValue but try a what about accented chars etc. or long names?

      function machineNameHandler(e) {
        var data = e.data;
        var options = data.options;
        var baseValue = $(e.target).val(); // <== replace SPACE by _ ?

        var rx = new RegExp(options.replace_pattern, 'g');
        var expected = baseValue.toLowerCase().replace(rx, options.replace).substr(0, options.maxlength);

        if (baseValue.toLowerCase() !== expected) { // <== triggers for SPACE , transliterations and long names.
          self.transliterate(baseValue, options).done(function (machine) {
            self.showMachineName(machine.substr(0, options.maxlength), data);
          });
        }
        else {
          self.showMachineName(expected, data);
        }
      }
clemens.tolboom’s picture

Status: Postponed (maintainer needs more info) » Active

Checking the network it calls TWICE for each keypress.

mradcliffe’s picture

I created a similar issue several months ago with a video demonstration. I'll mark that as a duplicate:

#2228985: Fix machine name UX regression when label/machine_name has a space

dsnopek’s picture

Issue summary: View changes

@mradcliffe: Whoa, awesome, added your video and screenshot to this issue! Thanks! :-)

dsnopek’s picture

Possible solutions:

  1. Wait until the user hasn't been typing for 1 second before making the AJAX request
  2. If the user types before the AJAX request has returned, abort it and start a new one (rather than starting so many AJAX requests at once)
  3. All of the above
clemens.tolboom’s picture

@dsnopek

Please update the issue summary by applying the template from http://drupal.org/node/1155816.

Maybe update the title too. Note this not only happens typing spaces but equally well when type !@#$%^&*() or accented and 'foreign' characters.

dsnopek’s picture

Title: JavaScript for #type => 'machine_name' registers key presses on 'source' slowly later, when label has spaces in it » JavaScript for #type => 'machine_name' registers key presses on 'source' slowly later, when label has spaces, special or international characters in it
Issue summary: View changes

Issue summary updated.

dsnopek’s picture

Issue summary: View changes

Heh, 'u' is not an international character. :-) Fixed issue summary.

dsnopek’s picture

Issue summary: View changes

Made header heirarchy make sense in issue summary.

dsnopek’s picture

Assigned: Unassigned » dsnopek

I'm going to take a stab at making a patch - I've written JavaScript to do the things I mentioned in #7 loads of times.

dsnopek’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.78 KB

Attached is a patch that:

  1. Waits until the user hasn't typed for 300 milliseconds before updating the machine name (does this if it needs to make an AJAX request or not, so that the behaviour is consistent from the user's perspective)
  2. Only does a single AJAX request at a time. If the user changes the label while the last AJAX request is pending, it cancels it before starting a new one.

Please let me know what you think!

mradcliffe’s picture

Issue tags: +Needs manual testing

Just adding a tag for manual testing.

dsnopek’s picture

Issue summary: View changes

Updated the User Interface and API changes settings in the issue summary.

andypost’s picture

littledynamo’s picture

Issue tags: +Amsterdam2014

Manual test of patch:

Without Patch

I originally tested the behaviour without the patch, using the steps to reproduce. While I didn't experience a 20s delay, I did find the display of the machine name to be jerky and rather jarring. It seemed like each letter was being added then removed then added again, which caused the layout to expand/collapse slightly with each new letter.

With Patch

I then applied the patch and found the experience to be much more smooth. When typing quickly, the entire machine name appeared after I stopped typing. When typing slowly, the machine name appeared in stages.

Overall, a more pleasant experience with the patch applied.

dsnopek’s picture

Issue summary: View changes

Updated the issue summary per discussion with @YesCT at BADCamp sprint.

mradcliffe’s picture

Issue summary: View changes
Issue tags: -Needs manual testing, -Amsterdam2014

Re-tested patch tonight. It works better. It's not as good as the Drupal 7 UX where you could see what you were typing in near real time. It would be nice to be able to return to the prior behavior, but that may not be possible (?).

This patch is a prioritized change, and will have the following impact:

  1. Work around the UX regression
  2. Make Drupal 8 fields usable to site builders.
  3. Let sales people and trainers show off Drupal 8's Field UI without being embarrassed at it's regression
  4. Increase core machine name usage by contrib developers who would otherwise be put off by the terrible UX

The patch is not an API change, but a behavior change. I don't think anyone would be calling the Drupal behavior by itself. So, no disruption.

mradcliffe’s picture

A couple of documentation tweaks.

  1. +++ b/core/misc/machine-name.js
    @@ -47,14 +49,29 @@
    +        // Abort the last pending request. Since the label has changed and it is
    

    The full stop seems awkward as I read it in my mind.

    "Abort the last pending request because the label has changed and it is no longer valid."

  2. +++ b/core/misc/machine-name.js
    @@ -47,14 +49,29 @@
    +        // Wait until it's been 300 milliseconds since the last event (ie. after
    

    id est is abbreviated i.e..

    It looks like the code style of being enclosed in parantheses is used throughout core, but i.e., appears more. Usually at the end of the sentence. The documentation standards page uses i.e. enclosed in a parantheses and makes no mention of preference.

    Perhaps

    "Wait 300 milliseconds since the last event to update the machine name i.e., after the user has stopped typing."

    or

    "Wait 300 milliseconds since the last event to update the machine name (i.e. after the user has stopped typing)."

    That's a lot of bikeshedwords for an abbreviation.

dsnopek’s picture

Issue summary: View changes
FileSize
1.76 KB
1.03 KB

@mradcliffe: Thanks for the review!

It's not as good as the Drupal 7 UX where you could see what you were typing in near real time. It would be nice to be able to return to the prior behavior, but that may not be possible (?).

So, current Drupal 8 HEAD already changes this away from the way it worked in Drupal 7, because it's making an AJAX call to transliterate the machine name. This patch only fixes that functionality so that it behaves sanely. I think discussing whether or not this AJAX call is a good idea should be in another issue - this issue is just fixing a bug in already merged functionailty.

A couple of documentation tweaks.

Thanks! I updated my patch for your comment suggestions. :-)

dsnopek’s picture

Issue summary: View changes

Added link from issue summary to my comment about the change in behavior from Drupal 7.

akalata’s picture

The updated patch in #21 still works correctly as described in #17.

dsnopek’s picture

Issue summary: View changes

Updated issue summary per @YesCT's advise at BADCamp.

akalata’s picture

Status: Needs review » Reviewed & tested by the community

Performing a manual code review, I used git diff -b to create a diff that ignores formatting changes, to reduce the "noise" in the full patch from existing lines that changed indent depth. Coding style conforms to Drupal standards and there are no out-of-scope changes.

jessebeach’s picture

Status: Reviewed & tested by the community » Needs work

This looks like a great use case for drupal.debounce. It'll remove the need to manage the timers in this little piece of code. I discussed with dsnopek and akalata in person at BAD Camp. dsnopek is rerolling now.

jessebeach’s picture

Status: Needs work » Reviewed & tested by the community

dsnopek and I walked through the solution I'd thought would be better. Turns out my suggestion was just more code and sacrificed readability, so after trying out an alternative, I'm fine with the code in this patch. Back to RTBC.

alexpott’s picture

Issue tags: +JavaScript

Tagging.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
eslint ./

core/misc/machine-name.js
  54:34  error  Expected '!==' and instead saw '!='  eqeqeq

✖ 1 problem
+++ b/core/misc/machine-name.js
@@ -47,14 +49,29 @@
+        if (xhr && xhr.readystate != 4) {

This passes if xhr.readystate !== 4

dsnopek’s picture

Status: Needs work » Needs review
FileSize
1.76 KB
438 bytes

Ok! Here is an updated patch which changes != to !==.

mradcliffe’s picture

Status: Needs review » Reviewed & tested by the community

Manually applied interdiff to my branch that had 11/6/2014 patch applied. Ran eslint.
Manually applied current patch and ran eslint.

Confirmed expected result in #29.

mradcliffe’s picture

Took a stab at using this new commit message thing

Edit: the credit & commit fieldset.

YesCT’s picture

@mradcliffe the commit credit fieldset is just a UI that makes a message there... It wont get saved for later use (by someone else, say a committer). :)
If you think a custom commit message is needed, then you could use that fieldset to generate a suggested commit message, and copy it in to the summary for consideration by a committer.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 8a9a3b7 and pushed to 8.0.x. Thanks!

  • alexpott committed 8a9a3b7 on 8.0.x
    Issue #2333053 by dsnopek, mradcliffe: JavaScript for #type => '...
dsnopek’s picture

Woohoo! Thanks, @alexpott for the commit! And thanks to everyone else: @clemens.tolboom for finding the source of the problem, @akalata and @littledynamo for the help testing, @mradcliffe for helping test AND the sweet video and screenshot, @jessebeach for walking through the code with me at BADCamp, and @YesCT for all the advice on how to improve the issue and push it forward. It seriously takes a lot of people to take an issue from inception to commit! :-)

Status: Fixed » Closed (fixed)

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

Alan D.’s picture

Only just saw this issue after posting another new issue related to how slow this widget has become. Within 10 minutes of working seriously in a client site using D8, I had 3 Machine name required errors and 2 incomplete machine names that I noticed and had to delete and redo these.

Cross-pointing any interested parties in case I've created a duplicate issues or if they're interested in finding a fix. Talking to the guy's in the office, they are all moaning about this slightly, and starting to simply manually type these.