NEW 26139
Implement onredo and onundo event handlers 
https://bugs.webkit.org/show_bug.cgi?id=26139
Summary Implement onredo and onundo event handlers
Sam Weinig
Reported 2009-06-02 10:41:57 PDT
We should implement onredo and onundo event handlers from HTML5.
Attachments
Patch (10.72 KB, patch)
2012-01-03 04:40 PST, sknikam 		rniwa: review-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
sknikam
Comment 1 2012-01-03 04:40:22 PST
Created attachment 120933 [details] Patch
sknikam
Comment 2 2012-01-03 05:00:27 PST
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.
Peter Beverloo
Comment 3 2012-01-03 08:00:49 PST
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.
Ryosuke Niwa
Comment 4 2012-01-03 09:38:13 PST
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.
Ryosuke Niwa
Comment 5 2012-01-03 09:41:34 PST
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.
sknikam
Comment 6 2012-01-03 22:35:45 PST
(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?
Ryosuke Niwa
Comment 7 2012-01-03 23:38:16 PST
(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.
Note You need to log in before you can comment on or make changes to this bug.