I'm going through all the JUnit test sequences that Scott and I came up with right now. We really have to land these two patches together -- if we don't, then all the Event.trigger() methods are only going to cause everyone to run headlong into the same problem I did. I'll be sending an updated patch Real Soon Now.
On Wed, Dec 10, 2008 at 9:10 PM, Scott Blum <[email protected]> wrote: > Update: this is still in progress, but more complicated than it seems. I > might look at ways to get the main patch in without the JUnit stuff. > > > On Wed, Dec 10, 2008 at 1:24 PM, Joel Webber <[email protected]> wrote: > >> Thanks. Still waiting on scottb to review my JUnit patch, then I will >> commit. >> >> >> On Mon, Dec 8, 2008 at 4:58 PM, Ray Ryan <[email protected]> wrote: >> >>> LGTM >>> Rietveld can't show inter-diff diffs, so I can't see, say, your version 1 >>> and comments on the left and the new patch on the right. That's a shame. >>> >>> rjrjr >>> >>> >>> On Tue, Dec 9, 2008 at 7:50 AM, Joel Webber <[email protected]> wrote: >>> >>>> Don't know why I can't attach another patch set to that codereview >>>> instance, so here it is. Maybe you can attach it (or maybe I'm just dumb >>>> and >>>> can't figure it out). >>>> >>>> >>>> On Mon, Dec 8, 2008 at 3:48 PM, <[email protected]> wrote: >>>> >>>>> 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 -~----------~----~----~----~------~----~------~--~---
