-
Notifications
You must be signed in to change notification settings - Fork 527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Re-introduce CodeMirror editor + delay setting failed checks until editor results have updated #2917
Conversation
* refactor: Add EditFieldHandle and EditorResult as interfaces for the editor value * Replace editor <textarea> with CodeMirror v6
Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>
I would test a couple of strings, that I remember I was unable to approve. But I cant find them on https://mozilla-pontoon-staging.herokuapp.com/ |
The two strings @Property syntax descriptor ‘%S’ contains components without a pipe between them. and %s search engine are now on https://mozilla-pontoon-staging.herokuapp.com/ |
@Joergen Try again now? We'd missed a step when updating the data. |
I have unapproved and reapproved the strings @Property syntax descriptor ‘%S’ contains components without a pipe between them., %s search engine and a couple other strings that will show warnings when approved. |
I approved a bunch of strings on the staging server, and everything works as it should :) |
Thank you @Joergen and @jakobkappel! We'll try to land this again, hopefully with more success this time. |
Fixes #2885, if verified by tests on staging. @jakobkappel & @Joergen, would you be able to check if the problems you saw show up with the Pontoon instance at mozilla-pontoon-staging.herokuapp.com? You should be able to log in using your usual credentials. Any actions made on staging are not reflected in any actual product, it's our testing ground.
This includes the changes from #2866, #2879, and #2884, and reverts those of #2890. On top of those there's d3c9bcf which should fix the problem.
Thanks to the video posted by @jakobkappel in #2885 (comment), I was able to start tracing this back to its source. The significant parts were:
%s search engine
) and target (Søgetjenesten %s
) pair of strings.What's happening here is that the suggested string will contain warnings (Simple capitalization, Starting capitalization, Starting punctuation) that should be shown, allowing a translator to then "Approve anyway". Because an approval can be triggered not just from the big "Approve" button but also from the smaller round buttons next to the history entries, we need to set the editor value when we have failed checks to show for it. This in turn updates the editor result, which we're taking as a signal to reset the failed checks and mark the message as having unsaved changes. And so the approval fails to go through, and the failed checks are dismissed before they're even shown.
Previously this worked because the action to reset the failed checks was conditional on the message already having unsaved changes, and in this particular case that state was only updated just after the failed checks would be reset. We can't really apply the same logic any more, because the unsaved changes state is now only determined when it's actually needed, rather than eagerly whenever it could change.
So instead the best solution for now is to delay setting the failed checks by one event cycle (with
setTimeout()
), which lets them happen after the result changes trigger the reset. I've also added some line comments to clarify this.Setting up an automated test case to simulate all this would be rather cumbersome.