Page MenuHomePhabricator

VisualEditor: Save button should be disabled if dm state is identical to page when it was loaded
Closed, ResolvedPublic


Right now, the Save button starts disabled, but when the transaction depth is non-zero it's enabled. This means that two actions that reverse each other (rather than being formally undone) let the user make a 'null edit', which MW then silently accepts.

Ideally, the Save/Create button should be disabled if dm state is identical to page when it was loaded (except for the special-case of oldid pages, where it should always be enabled). Roan suggests a string comparison of the current and original HTML is the least-bad option, given the lack of hash functions in JavaScript.

Version: unspecified
Severity: normal
See Also:



Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 1:03 AM
bzimport set Reference to bz42939.

Right now the Save button becomes enabled after the first transaction is applied and never goes back to disabled, even if the first transaction is undone. While this isn't ideal, doing a full HTML comparison on pretty much every keystroke may be a bit of an unnecessary overhead. Even doing an transaction stack comparison is a little unnecessary.

The real problem here is the creation of null edits and that only needs to be addressed when the save button is pressed. This could trigger an alert ("You haven't made any changes") and reset the button to disabled. We should also run this check in the window.onbeforeunload handler so people can leave the page nag-free if they undo their edits.

Agree on the window.onbeforeunload.

Not totally enamoured with the idea of a modal alert telling users off for behaviour that is currently silently failing (in core wikitext editor, if you make a null save it'll look like it saved, but no new entry in the history log).

Possibly we could over-ride the edit-success box to say "You made no changes so nothing was saved" or something similar? We should probably talk to E3 to see if this is something they'd want to see happen in core anyway...

After some more investigation is appears that comparing html might not do the trick. Doing something trivial like typing and deleting may not leave the html in it's original state. We currently have change markers (although these are queued up for deletion), but also hidden things like pawn characters and maybe other things in the future.

We can still implement the transaction stack check which we don't have at the moment, but telling you if there is a non-zero diff may have to be a job for Parsoid.

Fixed the zero-history case, and the onbeforeunload issue:

I'd suggest that once that change has been merged this bug can be closed.