Bug 224318 – Web Inspector: Audit Tab: Edits are not committed when leaving edit mode unless you first click into another text field

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

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.

[Radar WebKit Bug Importer]

<rdar://problem/76382755>

[Nikita Vasilyev]

https://github.com/WebKit/WebKit/blob/main/Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js#L96-L98

"blur" event doesn't happen when we stop editing.

[Nikita Vasilyev]

Created attachment 425541 [details]
Patch

[Devin Rousso]

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"`?

[Nikita Vasilyev]

Created attachment 425543 [details]
[Video] With patch applied

[Nikita Vasilyev]

(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.

[Devin Rousso]

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.).

[Nikita Vasilyev]

Created attachment 425812 [details]
Patch

In this patch, I'm listening to `WI.AuditManager.Event.EditingChanged` instead of creating a new event.

[Devin Rousso]

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)

[Nikita Vasilyev]

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!

[Nikita Vasilyev]

Created attachment 425924 [details]
Patch

[Devin Rousso]

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.

[Nikita Vasilyev]

Created attachment 426253 [details]
Patch

[Devin Rousso]

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)

[Nikita Vasilyev]

Created attachment 426257 [details]
Patch

[Devin Rousso]

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)`.

[Nikita Vasilyev]

Created attachment 426261 [details]
Patch

[EWS]

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].