| Quiddity | |
| Feb 25 2025, 10:56 PM |
| F58494814: simplescreenrecorder-2025-02-25_14.54.00.webm | |
| Feb 25 2025, 10:56 PM |
Steps to replicate the issue (include links if applicable):
What happens?:
What should have happened instead?:
Other information:
Tested and confirmed in both Firefox and Chromium, in two accounts, at
Change #1122687 had a related patch set uploaded (by MusikAnimal; author: MusikAnimal):
[mediawiki/extensions/CodeMirror@master] Revert "Introduce CodeMirror#addMwHook to deduplicate mw.hook handlers"
Change #1122688 had a related patch set uploaded (by MusikAnimal; author: MusikAnimal):
[mediawiki/extensions/CodeMirror@master] Revert "CodeMirror: add jQuery valHook so that .val() routes to CodeMirror"
Change #1122689 had a related patch set uploaded (by MusikAnimal; author: MusikAnimal):
[mediawiki/extensions/CodeMirror@master] Revert "CodeMirror: finalize stable interface for consumption by integrations"
Change #1122687 abandoned by MusikAnimal:
[mediawiki/extensions/CodeMirror@master] Revert "Introduce CodeMirror#addMwHook to deduplicate mw.hook handlers"
Reason:
squashed into I75015085f0aaf27dbcb635b88d5fab9bafeb02cb
Change #1122688 abandoned by MusikAnimal:
[mediawiki/extensions/CodeMirror@master] Revert "CodeMirror: add jQuery valHook so that .val() routes to CodeMirror"
Reason:
squashed into I75015085f0aaf27dbcb635b88d5fab9bafeb02cb
Change #1122696 had a related patch set uploaded (by MusikAnimal; author: MusikAnimal):
[mediawiki/extensions/CodeMirror@master] CodeMirror: use the EditorView's state property on form submission
Change #1122697 had a related patch set uploaded (by Tim Starling; author: MusikAnimal):
[mediawiki/extensions/CodeMirror@wmf/1.44.0-wmf.18] CodeMirror: use the EditorView's state property on form submission
Change #1122697 merged by jenkins-bot:
[mediawiki/extensions/CodeMirror@wmf/1.44.0-wmf.18] CodeMirror: use the EditorView's state property on form submission
Mentioned in SAL (#wikimedia-operations) [2025-02-26T00:44:45Z] <tstarling@deploy2002> Started scap sync-world: Backport for [[gerrit:1122697|CodeMirror: use the EditorView's state property on form submission (T387253)]]
Change #1122696 merged by Tim Starling:
[mediawiki/extensions/CodeMirror@master] CodeMirror: use the EditorView's state property on form submission
Mentioned in SAL (#wikimedia-operations) [2025-02-26T00:47:48Z] <tstarling@deploy2002> tstarling: Backport for [[gerrit:1122697|CodeMirror: use the EditorView's state property on form submission (T387253)]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)
Change #1122689 abandoned by MusikAnimal:
[mediawiki/extensions/CodeMirror@master] Revert changes since and including "finalize stable interface"
Reason:
fixed in Ieb52d589bc82
Mentioned in SAL (#wikimedia-operations) [2025-02-26T00:57:47Z] <tstarling@deploy2002> Finished scap sync-world: Backport for [[gerrit:1122697|CodeMirror: use the EditorView's state property on form submission (T387253)]] (duration: 13m 02s)
Thanks to @tstarling for the quick assistance! I ended up being able to fix the actual bug, so we deployed that instead of the revert. This task is no longer blocking wmf.18.
I've got one more related patch to follow (doesn't need deployment) so I'm leaving this open for the time being.
Change #1122716 had a related patch set uploaded (by MusikAnimal; author: MusikAnimal):
[mediawiki/extensions/CodeMirror@master] CodeMirror: don't persist EditorState after deactivation
r1122716 polishes off what the hotfix started, and adds more test coverage.
For whomever may be interested, here's a technical postmortem:
What happened?
r1108354 was a large refactor that was merged this past week. It turned what was previously a subtle yet harmless bug into an Unbreak-Now! by preventing changes made in CodeMirror from being copied back to the textarea when the edit form was submitted. This meant any edits made were instantly lost upon hitting "Publish changes" 😦 (It is possible Edit Recovery did it's thing for some users, but I can't confirm this). This did not effect the VisualEditor-MediaWiki-2017WikitextEditor, but would have had the following patch also been merged.
Why did it happen?
First, why the refactor? Well, all the integrations (such as WikiEditor (2010) and VisualEditor-MediaWiki-2017WikitextEditor) feature a "toggle" to turn CodeMirror on and off, yet each had their own implementation for handling that. The refactor was meant to put shared logic in one place, and provide a more consistent API across these integrations. It also worked under the idea that we don't need to reconstruct a whole new CodeMirror instance when toggling, rather only update the existing one.
And the bug: Since the beginning of the CodeMirror 6 upgrade project, our CodeMirror class had a shorthand property called state that was intended to point to the current EditorState – essentially the data model for the CodeMirror editor. this.state in this context was meant to be equivalent to this.view.state; however as it turns out the upstream library does not update the original state object when transactions are dispatched (such as typing). Rather, a new instance is assigned and should be accessed via the EditorView#state computed property. Before this wasn't a problem because we only ever used our cached state property as a means to determine whether CodeMirror was currently "active", given we also cleared it during deactivation. But we also tried to use this.state in fetching the contents in the form submission handler, which meant it used an outdated copy!
An interesting bug to say the least.
What have we done to prevent this moving forward?
Aside from the fixing the bug (1, 2), there are multiple new tests in place to check that CodeMirror changes are properly copied back to the original textarea.
Change #1122716 merged by jenkins-bot:
[mediawiki/extensions/CodeMirror@master] CodeMirror: don't persist EditorState after deactivation