For reasons not clear to me, I can't seem to add an updated patch set. I'll send it to the gwt-contrib thread and hope you can do it :)
http://codereview.appspot.com/8894/diff/1/9 File user/src/com/google/gwt/user/client/Event.java (right): http://codereview.appspot.com/8894/diff/1/9#newcode287 Line 287: * @param detail the event's detail property On 2008/12/05 00:13:15, rjrjr wrote: > Any example of valid values of detail? Detail is one of those weird fields that means different things for different events. It's defined as part of the DOM spec (in other words, if you don't know what it is, you probably won't care). http://codereview.appspot.com/8894/diff/1/7 File user/src/com/google/gwt/user/client/impl/DOMImplIE6.java (right): http://codereview.appspot.com/8894/diff/1/7#newcode230 Line 230: evt.relatedTarget = relatedTarget; On 2008/12/05 00:13:15, rjrjr wrote: > (Pretending I know what I'm talking about.) relatedTarget is a real field in > correct browsers, right? Is it a bit confusing to use it here in IE, where it > normally doesn't exist? Could we call this gwtTarget instead? Well, we're actually synthesizing a value that *is* identical to relatedTarget on other browsers (and that IE doesn't implement), so it doesn't seem unreasonable to simply synthesize it here. In the long run, it would make more sense to shift the public APIs to use 'relatedTarget', but that's probably a task for another day. http://codereview.appspot.com/8894/diff/1/10 File user/src/com/google/gwt/user/client/ui/CustomButton.java (right): http://codereview.appspot.com/8894/diff/1/10#newcode746 Line 746: // TODO(jgw): fill in these parameters somehow. On 2008/12/05 00:13:15, rjrjr wrote: > If this is actually possible, could you file an issue on it and point to the > issue here? Also, do we usually put our initials in our TODOs? (I hope so, but > thought I got scolded for doing so.) Whoops, I didn't mean to leave this TODO in there. I've changed this comment to explain why those values are unavailable (this shouldn't matter in practice, as click events often don't have useful mouse coordinates). http://codereview.appspot.com/8894/diff/1/4 File user/test/com/google/gwt/user/client/ui/CustomButtonTest.java (right): http://codereview.appspot.com/8894/diff/1/4#newcode178 Line 178: assertEquals("Expecting one click event", "click", events.get(0)); On 2008/12/05 00:13:15, rjrjr wrote: > I guess you're doing all three (over, down, up) b/c triggerClickEvent can't be > relied upon to do so? You might document that. Actually, this is intended to test that this series of events should *cause* CustomButton to fire a synthetic click event. I've added documentation to that effect. http://codereview.appspot.com/8894/diff/1/3 File user/test/com/google/gwt/user/client/ui/TriggerEventTest.java (right): http://codereview.appspot.com/8894/diff/1/3#newcode30 Line 30: public class TriggerEventTest extends GWTTestCase { On 2008/12/05 00:13:15, rjrjr wrote: > A thing of beauty. I have to take your word on which events bubble and which > don't. But then again I suppose I have the word of the tests themselves. :-) > > Should you be testing capture as well? Or does GWT stay clear of that? We don't do event capture, because there's really no reliable way to synthesize it on IE (it's sort of possible, but there are all kinds of edge cases that can't be made to work sensibly). http://codereview.appspot.com/8894/diff/1/3#newcode57 Line 57: assertTrue("Expecting child to receive the event before parent", On 2008/12/05 00:13:15, rjrjr wrote: > "Expecting parent to receive the event after the child", to avoid a dup message Done. http://codereview.appspot.com/8894/diff/1/3#newcode157 Line 157: init(); On 2008/12/05 00:13:15, rjrjr wrote: > just call this from gwtSetup rather than in each method Whoops, thanks. That's a lot simpler, isn't it? http://codereview.appspot.com/8894/diff/1/3#newcode166 Line 166: Event.triggerClickEvent(child, 0, 0, 0, 0, 0, false, false, false, false); On 2008/12/05 00:13:15, rjrjr wrote: > Seems like you should test all three core event types here. Good point. Just refactored the test to do click, keypress, and focus. http://codereview.appspot.com/8894/diff/1/3#newcode177 Line 177: public void onBrowserEvent(Event event) { On 2008/12/05 00:13:15, rjrjr wrote: > Seems like you could provide default implementations of onBrowserEvent that do > assertEquals(eventType, event.getType()), and get rid of a good chunk of these > anon classes. Perhaps even put it in a shared superclass. Fair enough. I've gone through and factored out a few helper classes to simplify these cases. There are still a few anonymous inners, but it's not as ugly as before. http://codereview.appspot.com/8894/diff/1/3#newcode233 Line 233: assertTrue("Expected parent to receive event", listener.parentReceived); On 2008/12/05 00:13:15, rjrjr wrote: > shouldn't you be checking the values of the various fields as well? You're > proving SCREEN_X et al, but I don't see that we know they're actually coming > through. Ditto below. These values are checked in assertMouseCoordinates() and assertAllShiftKeysOn(). http://codereview.appspot.com/8894/diff/1/3#newcode275 Line 275: assertTrue("Expecting click or dblclick", On 2008/12/05 00:13:15, rjrjr wrote: > no, you're just expecting a dblclick, and this should be an assertEquals (or > just a call to super, eh?) Cleaned up a bit with previously-mentioned refactoring. FWIW, that assertion was meant to imply that either a click or a dblclick could have been safely received, but it made more sense in a previous form. http://codereview.appspot.com/8894/diff/1/3#newcode307 Line 307: Event.triggerErrorEvent(child); On 2008/12/05 00:13:15, rjrjr wrote: > wouldn't this make more sense on the img? Can input elements every really > receive an error event? Good point. This and testTriggerLoadEvent() both reference the img element now. http://codereview.appspot.com/8894 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
