We should implement onredo and onundo event handlers from HTML5.
Created attachment 120933 [details] Patch
This patch is not ready for review yet. I tested the changes locally with the webkit test browser using attached text file b26139.html file. Only the 'undo' event is getting fired. The 'redo' event is not getting fired. While I dig further, I would appreciate any feedback and comment on the existing patch. This is my first webkit patch.
Comment on attachment 120933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120933&action=review Just some drive-by nits, thanks! > ChangeLog:10 > + No need to edit this file, it are (presumably local) svn:ignore changes irrelevant to this patch. > Source/JavaScriptCore/ChangeLog:11 > + Dito. > Source/WebCore/ChangeLog:11 > + * dom: Added property svn:ignore. Dito with the svn:ignore items in this list. > Source/WebKit/gtk/ChangeLog:9 > + This ChangeLog also only contains an svn:ignore property change. > LayoutTests/ChangeLog:3 > + https://bugs.webkit.org/show_bug.cgi?id=26139 It'd be great if you could include a title here. As a nit for the filename: I would personally prefer a name such as "onundo-onredo-events.html" over "b26139.html" and refer to the bug in an HTML comment within the test-case. > LayoutTests/editing/undo/b26139.html:34 > + Using window.execCommand it should be possible to make an automated test for this.
Comment on attachment 120933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120933&action=review > LayoutTests/editing/undo/b26139.html:3 > + > +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> > +<html xmlns="http://www.w3.org/1999/xhtml"> This is definitely not a XHTML 1.0 as onundo/onredo content attributes are not defined in XHTML1.0. > LayoutTests/editing/undo/b26139.html:15 > + function() > + { > + alert( 'The "redo" event has been fired!' ); > + }); Wrong indentation. > LayoutTests/editing/undo/b26139.html:22 > + function() > + { > + alert( 'The "undo" event has been fired!' ); > + }); > + Ditto. > LayoutTests/editing/undo/b26139.html:30 > +RandomText: <input type="text" name="randomText" /><br /> This test doesn't do anything. And you're also not adding an expected result. r- due to this. Please see http://trac.webkit.org/wiki#LayoutTests.
By the way, you also need to refer to a relevant spec that specifies the behavior. But as far as I know, the latest HTML5 editor's draft has removed definitions of undo/redo events and instead refers to http://rniwa.com/editing/undomanager.html However, my specification doesn't define onundo/onredo content attribute on any element at all. So you need some justification as to why you're adding onundo/onredo content attributes.
(In reply to comment #5) > By the way, you also need to refer to a relevant spec that specifies the behavior. But as far as I know, the latest HTML5 editor's draft has removed definitions of undo/redo events and instead refers to http://rniwa.com/editing/undomanager.html However, my specification doesn't define onundo/onredo content attribute on any element at all. So you need some justification as to why you're adding onundo/onredo content attributes. Ryosuke, I agree, I don't think my patch meets the requirements as per your spec. Is there a separate bug/workItem tracking the implementation of your spec?
(In reply to comment #6) > I agree, I don't think my patch meets the requirements as per your spec. > Is there a separate bug/workItem tracking the implementation of your spec? Not yet. I'm planning to work on the implementation this quarter though.