Bug 224318 - Web Inspector: Audit Tab: Edits are not committed when leaving edit mode unless you first click into another text field
Summary: Web Inspector: Audit Tab: Edits are not committed when leaving edit mode unle...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-07 22:25 PDT by Patrick Angle
Modified: 2021-04-26 14:54 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.56 KB, patch)
2021-04-08 14:21 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Video] With patch applied (795.69 KB, video/quicktime)
2021-04-08 14:26 PDT, Nikita Vasilyev
no flags Details
Patch (5.81 KB, patch)
2021-04-12 17:09 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (4.79 KB, patch)
2021-04-13 15:36 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (7.42 KB, patch)
2021-04-16 11:37 PDT, Nikita Vasilyev
drousso: review+
drousso: commit-queue-
Details | Formatted Diff | Diff
Patch (7.23 KB, patch)
2021-04-16 12:27 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (7.33 KB, patch)
2021-04-16 13:13 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Angle 2021-04-07 22:25:03 PDT
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.
Comment 1 Radar WebKit Bug Importer 2021-04-07 22:25:23 PDT
<rdar://problem/76382755>
Comment 2 Nikita Vasilyev 2021-04-08 13:57:50 PDT
https://github.com/WebKit/WebKit/blob/main/Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js#L96-L98

"blur" event doesn't happen when we stop editing.
Comment 3 Nikita Vasilyev 2021-04-08 14:21:16 PDT
Created attachment 425541 [details]
Patch
Comment 4 Devin Rousso 2021-04-08 14:26:42 PDT
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"`?
Comment 5 Nikita Vasilyev 2021-04-08 14:26:50 PDT
Created attachment 425543 [details]
[Video] With patch applied
Comment 6 Nikita Vasilyev 2021-04-08 14:28:48 PDT
(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 7 Devin Rousso 2021-04-08 14:44:10 PDT
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.).
Comment 8 Nikita Vasilyev 2021-04-12 17:09:50 PDT
Created attachment 425812 [details]
Patch

In this patch, I'm listening to `WI.AuditManager.Event.EditingChanged` instead of creating a new event.
Comment 9 Devin Rousso 2021-04-12 17:17:29 PDT
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 10 Nikita Vasilyev 2021-04-13 15:36:29 PDT
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!
Comment 11 Nikita Vasilyev 2021-04-13 15:36:48 PDT
Created attachment 425924 [details]
Patch
Comment 12 Devin Rousso 2021-04-16 10:27:05 PDT
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.
Comment 13 Nikita Vasilyev 2021-04-16 11:37:27 PDT
Created attachment 426253 [details]
Patch
Comment 14 Devin Rousso 2021-04-16 12:12:02 PDT
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)
Comment 15 Nikita Vasilyev 2021-04-16 12:27:31 PDT
Created attachment 426257 [details]
Patch
Comment 16 Devin Rousso 2021-04-16 12:34:24 PDT
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)`.
Comment 17 Nikita Vasilyev 2021-04-16 13:13:20 PDT
Created attachment 426261 [details]
Patch
Comment 18 EWS 2021-04-26 14:54:30 PDT
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].