Steps to reproduce: 1. Open Web Inspector 2. Go to the Audit Tab 3. Enter Edit mode 4. Create a new test case 5. Change the test case's main function in some way 6. Click the Done button to leave edit mode 7. Return to edit mode 8. The changes are not present ☹️ Other notes: The audit is saved if I first click into edit the description of the audit, so I suspect we are saving when focus is lost, just not when we leave edit mode.
<rdar://problem/76382755>
https://github.com/WebKit/WebKit/blob/main/Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js#L96-L98 "blur" event doesn't happen when we stop editing.
Created attachment 425541 [details] Patch
Comment on attachment 425541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425541&action=review > Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:100 > + testCodeMirror.on("blur", saveTest); I think this is avoiding the underlying issue. Why is it that `"blur"` doesn't get dispatched when exiting Edit mode? Do we maybe instead need to add a `Debouncer` that's triggered on each `"change"`?
Created attachment 425543 [details] [Video] With patch applied
(In reply to Devin Rousso from comment #4) > Comment on attachment 425541 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425541&action=review > > > Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:100 > > + testCodeMirror.on("blur", saveTest); > > I think this is avoiding the underlying issue. Why is it that `"blur"` > doesn't get dispatched when exiting Edit mode? > > Do we maybe instead need to add a `Debouncer` that's triggered on each > `"change"`? Because "blur" doesn't get dispatched when an element is removed from the DOM. I don't think adding a debunker is better in any way. It still leaves a possibility of saving outdated version.
Comment on attachment 425541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425541&action=review >>> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:100 >>> + testCodeMirror.on("blur", saveTest); >> >> I think this is avoiding the underlying issue. Why is it that `"blur"` doesn't get dispatched when exiting Edit mode? >> >> Do we maybe instead need to add a `Debouncer` that's triggered on each `"change"`? > > Because "blur" doesn't get dispatched when an element is removed from the DOM. > I don't think adding a debunker is better in any way. It still leaves a possibility of saving outdated version. In that case why not save then changes when we're about to exit Edit mode (i.e. `WI.AuditManager.Event.EditingChanged`)? Probably on `WI.AuditTestContentView` since there's stuff in there too that might need to be saved. BTW we'd also likely need to do something similar for the "setup" editor too. I wonder what the performance of saving if on every `"change"` would be. We already do that for basically everything else (e.g. name, description, supports, etc.).
Created attachment 425812 [details] Patch In this patch, I'm listening to `WI.AuditManager.Event.EditingChanged` instead of creating a new event.
Comment on attachment 425812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425812&action=review > Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:99 > + this.dispatchEventToListeners(WI.AuditManager.Event.EditingChanged, {editing}); Can we have listeners just query `WI.auditManager.editing` instead? > Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:-103 > - console.assert(WI.auditManager.editing); this is unfortunate :( > Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:321 > + return; > + WI.auditManager.addEventListener(WI.AuditManager.Event.EditingChanged, this.handleAuditManagerEditingChanged, this); Style: I'd either add a newline or not make this early-return > Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:330 > + return; > + this._saveTest(); ditto (:320) > Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:343 > + handleAuditManagerEditingChanged(event) Can this be private? Or did you mean to add/remove the event listeners in the base `WI.AuditTestContentView` so that we can do something similar for `setupCodeMirror` too? Maybe have the event handler be private and have an extendable (not overridable) `saveEditedData` or something that this class can then extend to have the same logic as `_saveTest` inside of it. > Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:346 > + if (!event.data.editing) ditto (AuditManager.js:99)
Comment on attachment 425812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425812&action=review >> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:99 >> + this.dispatchEventToListeners(WI.AuditManager.Event.EditingChanged, {editing}); > > Can we have listeners just query `WI.auditManager.editing` instead? Yes, that makes sense. >> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:343 >> + handleAuditManagerEditingChanged(event) > > Can this be private? Or did you mean to add/remove the event listeners in the base `WI.AuditTestContentView` so that we can do something similar for `setupCodeMirror` too? Maybe have the event handler be private and have an extendable (not overridable) `saveEditedData` or something that this class can then extend to have the same logic as `_saveTest` inside of it. I meant this to be private!
Created attachment 425924 [details] Patch
Comment on attachment 425924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425924&action=review What about `setupCodeMirror`? > Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:325 > + super.detached(); please put the `super.*` call at the end of the function when dealing with destructor-like functions so that the superclass doesn't "invisibly" tear-down something that the subclass needs > Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:371 > + console.assert(this.representedObject); I don't think this check is necessary since we already check for this in the `constructor` and `representedObject` is intended to be set once and readonly after that.
Created attachment 426253 [details] Patch
Comment on attachment 426253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426253&action=review r=me > Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:329 > + if (!this.representedObject.editable) > + return; NIT: Rather than repeating this from `super.saveEditedData`, could we move this check to `_handleEditingChanged` so that we don't even call `saveEditedData` unless `this.representedObject.editable` (you could `console.assert` this inside `WI.AuditTestContentView`)? > Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:331 > + console.assert(this._testCodeMirror); NIT: IMO these kinds of `console.assert` really don't serve any purpose because if it fails we'd immediately throw an error when trying to use it on the next line. I think `console.assert` really are more useful for knowing things _about_ the object (e.g. checking that the parameter is of a particular type), not just that its truthy. I'd just drop it. > Source/WebInspectorUI/UserInterface/Views/AuditTestContentView.js:322 > + console.assert(this._setupCodeMirror); ditto (AuditTestCaseContentView.js:331)
Created attachment 426257 [details] Patch
Comment on attachment 426257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426257&action=review > Source/WebInspectorUI/UserInterface/Views/AuditTestContentView.js:290 > + if (this.representedObject.editable) Do you also want to check `WI.auditManager.editing` here too? > Source/WebInspectorUI/UserInterface/Views/AuditTestContentView.js:319 > + this.representedObject.setup = this._setupCodeMirror.getValue().trim(); I'd add an `console.assert(this.representedObject.editable, this.representedObject)`.
Created attachment 426261 [details] Patch
Committed r276616 (237045@main): <https://commits.webkit.org/237045@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426261 [details].