Save Button error

[numanWD]

You need to click 2 times in the Save button to be able to save the record.

The first time you click, just select the button but doesn't trigger the save action.

Instead the Save and close button works perfectly.

[kinglozzer]

Seems to be related to the changetracker. If you don’t make any changes, it’ll work first time

[jonom]

I may have come up with a way to reproduce this issue:

1. Open any page in the CMS
2. Open the security section of the CMS
3. Open any user
4. (Optional step) Click Save. Should work fine.
5. Type an extra letter in the text field that has focus (FirstName)
6. Click the Save button, without blurring the text field first by clicking anywhere else. Should only focus the field.
7. Click the save button again (Should trigger actual save)

If you reload the browser at this point everything will work fine again, but if you don't the bug persists. You can repeat steps 6 and 7 and will need two clicks every time.

So it appears that loading a page edit form is at least one way to introduce the conditions needed for this bug. Maybe it's overriding some entwine behaviour?

[unclecheese]

I think the issue is that the first click is to blur the input, and the second is to click the button. The same thing happens when editing pages in CMSMain.

[jonom]

@unclecheese the specific steps above only work when betterbuttons is installed, so assume something in this module is causing this behaviour when dealing with regular (non-versioned) save buttons.

Similar behaviour occurs when editing a page, with or without betterbuttons installed. In that case if you click the Save button before it has recognised that changes have taken place the state changes from: to

So perhaps the original bug is somewhere in framework/cms but betterbuttons somehow widens the scope of it?

Either way, being more aggressive about detecting changes might be all we need to do to fix it. You had a PR going a while back that might do the trick, any interest in rebasing it and finishing it off?

[unclecheese]

Alright, I can replicate it now. Thanks, Jono. Those are really specific steps you have to follow.

I've confirmed that the issue happens even when the BetterButtons javascript is blocked, so it must be something external. I have a strong suspicion that when a page is loaded in CMSMain, an entwine handler is attached to the form that provides the behaviour that changes the grey "Saved" button to the green "Save draft" button on blur, which is desirable. That same behaviour must be executing on the non-page forms due to some kind of common selector the better buttons are using.

Researching for the next 10 minutes.......

[unclecheese]

The issue is in CMSMain.EditForm.js (line 397):

        $('.cms-edit-form.changed').entwine({
            onmatch: function(e) {
                // Commenting these two lines out fixes it.
                this.find('button[name=action_save]').button('option', 'showingAlternate', true);
                this.find('button[name=action_publish]').button('option', 'showingAlternate', true);
                this._super(e);
            },
            onunmatch: function(e) {
                var saveButton = this.find('button[name=action_save]');
                if(saveButton.data('button')) saveButton.button('option', 'showingAlternate', false);
                var publishButton = this.find('button[name=action_publish]');
                if(publishButton.data('button')) publishButton.button('option', 'showingAlternate', false);
                this._super(e);
            }
        });

In theory, that behaviour should only apply to forms in CMSMain. If we wanted to be rockstars, we could have it apply to anything versioned, but versioned dataobjects are second class citizens, and I'm fine keeping it that way. It would be a nice-to-have, but if versioned dataobjects don't update the buttons on blur, I can live with it.

So, that leaves us with the option of sniffing out ModelAdmin, or, "not CMSMain," basically. At the moment there's no great way to do that. They both fall under body.cms and produce Form.cms-edit-form. We could have BetterButtons add a class to the form (e.g. form.better-buttons-form.cms-edit-form and then override the behaviour like so:

        $('form.better-buttons-form.cms-edit-form.changed').entwine({
            onmatch: function(e) { },
            onunmatch: function (e) {}
        });

However, that dangerously assumes that nothing is happening in the this._super(e) of the original code.

I'm going to have have a think on this one. Keen to hear your thoughts.

[jonom]

It seems like the root of the problem is that the change recognition and button click are happening at the same moment, so one behaviour is interfering with another. If you blur the field before clicking the Save button the code you referenced above works fine, I'm assuming because it allows plenty of time for the change recognition to execute before you click Save. So if we can get the change recognition to happen before a form field is blurred (i.e. while you're typing) I don't think we'd have a problem anymore, as it would kick in well before you click Save. So I'd say making the change detection kick on on keyup/paste etc. instead of just change (which equates to blur) might solve the whole problem. I should test the theory, just don't have any spare seconds right now :P

[unclecheese]

Yeah, the problem is the change detection is all handled by a thirdparty library. All the CMS implementation does is add .changed to the form, and the entwine handlers watch for that.

Anyway, this is resolved in https://github.com/unclecheese/silverstripe-gridfield-betterbuttons/commit/245caaff2abc95e7037d6160b79eae9f4e2926e5.

[jonom]

Yeah, the problem is the change detection is all handled by a thirdparty library

Ah, I see. That's a shame!

I took 245caaf for a spin, seems to do the trick. It doesn't fix the issue when editing a page (still need two clicks to save a page) but it fixes the Security scenario outlined above. Since it's only the latter one that is introduced by this module that seems appropriate.

Thanks!!

[unclecheese]

Yeah, the only thing that is going to change the behaviour in CMSMain is merging my PR from years ago that updates on keyup.

[jonom]

I'm keen to help push that through if you want to re-base on 3 branch :)

[unclecheese]

Ah, right. In the spirit of merging imperfect code, right? I'm still keen to polish it a bit, but let's just see how we get on with it as is..

[jonom]

Fair enough. I think that PR has a lot of merit besides just fixing a click behaviour though. Currently if you make a change to only one field on a page in the CMS the button state lies to you by indicating nothing has changed. So addressing that would be a usability win (as per the original intention of that PR).

[unclecheese]

Yeah, it's been a usability killer for as long as I can remember. Let's put an end to it. :)