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:
Proposed resolution
- Wait until the user hasn't been typing for 300 milliseconds before making the AJAX request
- 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 patchManual 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.
Comment | File | Size | Author |
---|---|---|---|
#30 | interdiff.txt | 438 bytes | dsnopek |
#30 | drupal8-machine-name-2333053-30.patch | 1.76 KB | dsnopek |
Comments
Comment #1
clemens.tolboomI 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.
Comment #2
dsnopekI 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...
Comment #3
clemens.tolboomI just learned this is due to callback to Drupal
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?
Comment #4
clemens.tolboomChecking the network it calls TWICE for each keypress.
Comment #5
mradcliffeI 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
Comment #6
dsnopek@mradcliffe: Whoa, awesome, added your video and screenshot to this issue! Thanks! :-)
Comment #7
dsnopekPossible solutions:
Comment #8
clemens.tolboom@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.
Comment #9
dsnopekIssue summary updated.
Comment #10
dsnopekHeh, 'u' is not an international character. :-) Fixed issue summary.
Comment #11
dsnopekMade header heirarchy make sense in issue summary.
Comment #12
dsnopekI'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.
Comment #13
dsnopekAttached is a patch that:
Please let me know what you think!
Comment #14
mradcliffeJust adding a tag for manual testing.
Comment #15
dsnopekUpdated the User Interface and API changes settings in the issue summary.
Comment #16
andypostComment #17
littledynamo CreditAttribution: littledynamo commentedManual 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.
Comment #18
dsnopekUpdated the issue summary per discussion with @YesCT at BADCamp sprint.
Comment #19
mradcliffeRe-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:
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.
Comment #20
mradcliffeA couple of documentation tweaks.
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."
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.Comment #21
dsnopek@mradcliffe: Thanks for the review!
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.
Thanks! I updated my patch for your comment suggestions. :-)
Comment #22
dsnopekAdded link from issue summary to my comment about the change in behavior from Drupal 7.
Comment #23
akalata CreditAttribution: akalata commentedThe updated patch in #21 still works correctly as described in #17.
Comment #24
dsnopekUpdated issue summary per @YesCT's advise at BADCamp.
Comment #25
akalata CreditAttribution: akalata commentedPerforming 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.Comment #26
jessebeach CreditAttribution: jessebeach commentedThis 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.Comment #27
jessebeach CreditAttribution: jessebeach commenteddsnopek 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.
Comment #28
alexpottTagging.
Comment #29
alexpottThis passes if
xhr.readystate !== 4
Comment #30
dsnopekOk! Here is an updated patch which changes
!=
to!==
.Comment #31
mradcliffeManually 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.
Comment #32
mradcliffeTook a stab at using this new commit message thing
Edit: the credit & commit fieldset.
Comment #33
YesCT CreditAttribution: YesCT commented@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.
Comment #34
alexpottThis 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!
Comment #36
dsnopekWoohoo! 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! :-)
Comment #38
Alan D. CreditAttribution: Alan D. commentedOnly 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.