Bug 190009 - Need a way for JavaScript (or bundle) code to participate in undo
Summary: Need a way for JavaScript (or bundle) code to participate in undo
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
: 193720 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-09-26 14:01 PDT by mitz
Modified: 2019-01-25 10:05 PST (History)
8 users (show)

See Also:


Attachments
Patch (37.61 KB, patch)
2019-01-23 09:49 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix WPE/GTK/Win (37.23 KB, patch)
2019-01-23 10:56 PST, Wenson Hsieh
rniwa: review+
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.73 MB, application/zip)
2019-01-23 12:04 PST, EWS Watchlist
no flags Details
Patch for EWS (42.09 KB, patch)
2019-01-25 07:31 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch for EWS (42.23 KB, patch)
2019-01-25 08:12 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2018-09-26 14:01:12 PDT
There needs to be a way for code in the Web Content process—presumably JavaScript code on the page, but possibly bundle code—to participate in undo.

Here is a concrete use case that should be supported: if an (undoable) editing operation triggers an event handler (such as an input event handler or DOM mutation event handler), then in the context of handling the event, there should be a way to register a handler to be run when the editing operation gets undone. There should probably also be a way to determine whether the event handler is running because of an undo/redo invocation.
Comment 1 mitz 2018-09-26 14:02:28 PDT
<rdar://problem/44807048>
Comment 2 Ryosuke Niwa 2018-09-28 03:13:11 PDT
See https://bugs.webkit.org/show_bug.cgi?id=87189 for the previous attempt.
Comment 3 Wenson Hsieh 2019-01-23 08:25:56 PST
*** Bug 193720 has been marked as a duplicate of this bug. ***
Comment 4 Wenson Hsieh 2019-01-23 09:49:06 PST
Created attachment 359897 [details]
Patch
Comment 5 Wenson Hsieh 2019-01-23 10:56:27 PST
Created attachment 359905 [details]
Fix WPE/GTK/Win
Comment 6 EWS Watchlist 2019-01-23 12:04:39 PST Comment hidden (obsolete)
Comment 7 EWS Watchlist 2019-01-23 12:04:41 PST Comment hidden (obsolete)
Comment 8 Ryosuke Niwa 2019-01-24 20:25:37 PST
Comment on attachment 359905 [details]
Fix WPE/GTK/Win

View in context: https://bugs.webkit.org/attachment.cgi?id=359905&action=review

> Source/WebCore/editing/CompositeEditCommand.h:92
> +    void invalidate() final { }

I don't like "invalidate". It doesn't tell us what it does, nor what we're invalidating, or what invalidating edit command means.
I think we should say something more specific like didRemoveFromUndoManager.

> Source/WebCore/page/UndoManager.cpp:48
> +        // FIXME: Support adding the same UndoItem to an UndoManager multiple times.

We probably want to throw in this case.

> Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:299
> +    readonly attribute DOMString undoActionName;
> +    readonly attribute DOMString redoActionName;

action name is Cocoa terminology. It's also somewhat surprising that there is only one name.
We should probably call these something like lastUndoLabel and firstRedoLabel.
Comment 9 Wenson Hsieh 2019-01-24 21:25:56 PST
Comment on attachment 359905 [details]
Fix WPE/GTK/Win

View in context: https://bugs.webkit.org/attachment.cgi?id=359905&action=review

>> Source/WebCore/editing/CompositeEditCommand.h:92
>> +    void invalidate() final { }
> 
> I don't like "invalidate". It doesn't tell us what it does, nor what we're invalidating, or what invalidating edit command means.
> I think we should say something more specific like didRemoveFromUndoManager.

Ok! Renamed this to didRemoveFromUndoManager().

>> Source/WebCore/page/UndoManager.cpp:48
>> +        // FIXME: Support adding the same UndoItem to an UndoManager multiple times.
> 
> We probably want to throw in this case.

Good call. I made UndoManager.addItem() capable of throwing exceptions, and added exceptions in the case of this early return, and the !frame check below. This also made me realize that this first check for `m_items.contains(item.ptr())` isn't quite right — we really want to bail if the given UndoItem already has already been added an undoManager, not just this undo manager in particular!

I added a new layout test as well, to verify that we throw an exception.

>> Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:299
>> +    readonly attribute DOMString redoActionName;
> 
> action name is Cocoa terminology. It's also somewhat surprising that there is only one name.
> We should probably call these something like lastUndoLabel and firstRedoLabel.

Sounds good! Renamed to lastUndoLabel and firstRedoLabel.
Comment 10 Wenson Hsieh 2019-01-25 07:31:18 PST
Created attachment 360113 [details]
Patch for EWS
Comment 11 Wenson Hsieh 2019-01-25 08:12:59 PST
Created attachment 360117 [details]
Patch for EWS
Comment 12 WebKit Commit Bot 2019-01-25 09:23:12 PST
Comment on attachment 360117 [details]
Patch for EWS

Clearing flags on attachment: 360117

Committed r240476: <https://trac.webkit.org/changeset/240476>