Announcement

Collapse
No announcement yet.

[CLOSED] [#461] [1.10] Generic (html5) Element and Event classes

Collapse
X
  • Filter
  • Time
  • Show
Clear All
new posts

    [CLOSED] [#461] [1.10] Generic (html5) Element and Event classes

    When an InputElement, for example, has its "onclick" event raised then this may be received by setting the "OnChange" property to an action of type "Action<Event>". The Event class has a "Target" property that indicates the html element that raised the event. In the case of an InputElement, we know that this Target will always be the InputElement itself, so it would be convenient to have the Target type be InputElement rather than the less specific Element class.

    I would suggest that one way to do this would be for Event to be made generic - ie. "Event<TSource>". In the example above, TSource would be InputElement.

    This would have some knock-on effects - it might be necessary for Element to also become generic (Element<TSource>) so that the OnChange, OnClick, etc.. properties are all of type Action<Event<TSource>>. Alternatively, the Element class could be left as it is and then classes such as InputElement could have OnChange, OnClick, etc.. properties marked as "new" -

    public class InputElement : Element
    {
        // .. other properties ..
        public new Action<Event<InputElement>> OnChange;
        // .. other properties ..
    }
    I think that any opportunity to allow the type system to be used to greater effect can be beneficial, but I'm also raising this feature after Daniil made some excellent comments on my Bridge / React post last month: Strongly-typed React (with Bridge.net) - it would be great to create "attributes" classes for the standard React DOM components based upon the classes that are in Bridge.Html5 but doing so, currently, would result in loss of type information as explained in the example above.

    Let me know if I can offer any more information, explanation or examples!

    #2
    Hi @ProductiveRage,

    Thank you for raising that in the forums! I will be investigating this in details.

    Comment


      #3
      Logged the feature request in GitHub.
      https://github.com/bridgedotnet/Bridge/issues/461

      There are CurrentTarget and Target properties which are not of the same type for every element and for every event.

      There are, for example, InputElement and DivElement. It looks like CurrentTarget and Target are going to have the same type (InputElement) for any InputElement's event (true?). But it is not true for sure for DivElement. For example, DivElement's OnClick. Clicking on a ParagraphElement inside a DivElement will result in Event.Target being a ParagraphElement, not a DivElement. Though, .CurrentTarget is a DivElement. So, for such elements like DivElement, the mouse-releated events (at least or only mouse events?) should have Event.Target as Element.

      The above things complicate it a little and I am trying to come up with a good solution for all the cases or, at least, for most.

      So far, I am thinking on the following prototype. Please have a look and share your opinion on that. Do you see/foresee any caveats in this model? Would this model suite your needs?

      I also think we will try to avoid using a new modifier on properties when possible.

      Event.cs
      /// <summary>
      /// The Event interface represents any event of the DOM. It contains common properties and methods to any event.
      /// </summary>
      /// <typeparam name="TCurrentTarget">The type of CurrentTarget</typeparam>
      /// <typeparam name="TTarget">The type of Target</typeparam>
      [Ignore]
      [Name("Event")]
      public class Event<TCurrentTarget, TTarget>
      {
          ... all the Event API members ...
      }
      
      /// <summary>
      /// The generic version of the Event class if CurrentTarget and Target have the same type (for example, InputElement's events).
      /// </summary>
      /// <typeparam name="T">The type of Target and CurrentTarget</typeparam>
      [Ignore]
      [Name("Event")]
      public class Event<T> : Event<T, T> { }
      
      /// <summary>
      /// The non-generic version of the Event class. CurrentTarget and Target are both of the Element type.
      /// </summary>
      [Ignore]
      [Name("Event")]
      public class Event : Event<Element> { }
      Element.cs
      /// <summary>
      /// The Element interface represents an object of a Document. This interface describes methods and properties common to all kinds of elements.
      /// Specific behaviors are described in interfaces which inherit from Element but add additional functionality.
      /// For example, the HTMLElement interface is the base interface for HTML elements, while the SVGElement interface is the basis for all SVG elements.
      /// </summary>
      /// <typeparam name="TEvent">The type of all Element's events except mouse events</typeparam>
      /// <typeparam name="TMouseEvent">The type of Element's mouse events</typeparam>
      [Ignore]
      [Name("HTMLElement")]
      public class Element<TEvent, TMouseEvent> : Node
      {
          ... all the Element API members ...
      }
      
      /// <summary>
      /// The non-generic Element class. It has the same Event type for all the events
      /// </summary>
      [Ignore]
      [Name("HTMLElement")]
      public class Element : Element<Event> { }
      
      /// <summary>
      /// The generic Element class
      /// </summary>
      /// <typeparam name="TEvent">The type of Element's events</typeparam>
      [Ignore]
      [Name("HTMLElement")]
      public class Element<TEvent> : Element<TEvent, TEvent> { }
      InputElement.cs
      /// <summary>
      /// The HTMLInputElement interface provides special properties and methods (beyond the regular HTMLElement interface it also has available to it by inheritance) for manipulating the layout and presentation of input elements.
      /// </summary>
      [Ignore]
      [Name("HTMLInputElement")]
      public class InputElement : Element<Event<InputElement>>
      {
          ... all the InputElement API members
      }
      DivElement.cs
      /// <summary>
      /// The HTMLDivElement interface provides special properties (beyond the regular HTMLElement interface it also has available to it by inheritance) for manipulating div elements.
      /// </summary>
      [Ignore]
      [Name("HTMLDivElement")]
      public class DivElement : Element<Event<DivElement>, Event<DivElement, Element>>
      {
          ... all the DivElement API members ...
      }
      Last edited by Daniil; 2015-09-29 @ 07:52 AM.

      Comment


        #4
        You're right, that does complicate things!

        In most cases, I think that it is probably the "CurrentTarget" property that is of most use - or, rather, it is most likely this property whose type is expected to be more well known. That might not be phrased very well.. I think that what I want to say is that since "CurrentTarget" will always be of the type of the current element (Input, Div, etc..) then that is the one we should worry about more. The "Target" property could be anything - the drag-related properties might be even more complicated than the other mouse properties (or at least as complicated).

        So maybe, if we're working on that basis, it might be better to have a simpler interface where we only have a single generic type parameter "TCurrentTarget" - eg.

        [Ignore]
        [Name("HTMLInputElement")]
        public class InputElement : Element<InputElement>
        {
            // ... all the InputElement API members
        }
        and

        [Ignore]
        [Name("HTMLDivElement")]
        public class DivElement : Element<DivElement>
        {
            // ... all the DivElementAPI members
        }
        and

        [Ignore]
        [Name("HTMLElement")]
        public class Element : Element<Element> { }
        
        [Ignore]
        [Name("HTMLElement")]
        public class Element<TCurrentTarget> : Node
        {
            // ... all the Element API members ...
            public System.Action<Event<TCurrentTarget>> OnChange;
            // .. other members ..
        }
        Do you think this all makes sense?

        That way, the simple cases such as hooking up an "OnChange" callback from an InputElement would benefit from the strong typing of the "CurrentTarget" property but the "Target" would remain as type "Element" since there are no stronger guarantees that may be made about it.

        I think that the element classes look tidier as

        public class InputElement : Element<InputElement>
        rather then

        public class InputElement : Element<Event<InputElement>>
        but I can see why you suggested it in the more verbose manner - it is a little more explicit, which is often good. It would be a judgement call as to whether the more explicit declaration is worth the added verbosity.

        Comment


          #5
          1. I agree that we should focus on CurrentTarget rather than Target. Fortunately, there is such a property in the Event API as CurrentTarget that we can rely on.

          2. I like your approach having Element<InputElement> rather than Element<Event<InputElement>>. Though, it is quite challenging to choose a specific one - mine or yours. Yes, your approach makes the Element classes look tidier, from other side Element<Element> or Element<InputElement> definitions look a little bit quirky. I am still in an indecisive state on that, but seems like there are bit more chances to pick your approach finally:)

          3. But... while being quite happy about coming closer to a feature prototype I found out some general problems with either approach.

          3.1. First of all, I've found that, for example, this class and a few similar ones stop compiling:
          public class OptionsCollection : HTMLCollection<OptionElement>
          with this error
          The type 'Bridge.Html5.OptionElement' cannot be used as type parameter 'T' in the generic type or method 'Bridge.Html5.HTMLCollection<T>'. There is no implicit reference conversion from 'Bridge.Html5.OptionElement' to 'Bridge.Html5.Element'.

          It is because of where T : Element in the HTMLCollection class definition:
          public class HTMLCollection<T> : IEnumerable<T> where T : Element

          The same problem happens with
          public static T GetElementById<T>(string id) where T : Element
          and I guess with any method or class with where T : Element in its definition. There is a few such classes and method in Bridge.Html5.

          In my understanding, the problem here is that the Element class is not a superclass anymore for all the other Element classes (InputElement, DivElement, etc.). Element<T> is now a superclass. At the moment I don't know what to replace where T : Element with in HTMLCollection<T>, GetElementById<T> and others. Hmmm!!! I was pretty sure I tried to replace with where T : Element<T> and it didn't work. But I gave it one more try and it seems worked. Maybe, it is not a problem then. Although, I am still not sure it is technically correct to replace where T : Element with where T : Element<T>. What do you think?

          3.2. Another problem what I found out is with a generic Event class.

          We have such extensions for the Event class:
          [Ignore]
          public static class EventsExtension
          {
              [Template("Bridge.is({0}, MouseEvent)")]
              public static bool IsMouseEvent(this Event e)
          
              ... Three other similar methods ...
          }
          This code continues working with a non-generic event, but, obviously, these method are not available for generic Event<TCurrentTarget> classes. So, this code stops compiling because ev is now a generic class:
          InputElement input = new InputElement();
          input.OnChange += (ev) =>
          {
              Console.Log(ev.IsMouseEvent());
          };
          A possible workaround could be Console.Log(ev.As<Event>().IsMouseEvent());, but I don't really consider it a solution. It would be a little bit dishonourably forcing a Bridge developer to use .As<Event> in such a case. Those methods must be available for generic and non-generic Event classes as well. Hopefully, without having to repeat those methods (IsMouseEvent and others) twice. Seems a solution is found with this appoach:
          public static bool IsMouseEvent<T>(this Event<T> e) where T : Element<T>

          3.3. Just found out problem more. This code stops working because of "cannot explicitly convert" InputElement and TextAreaElement to Element.
          Element element;
          element = new InputElement();
          element = new TextAreaElement();
          As I mentioned above the Element class is not a superclass anymore for all the Element classes, but Element<T> is. The Node class could be used instead because it is a superclass for both.
          Node element;
          element = new InputElement();
          element = new TextAreaElement();
          But I am not sure it is good and at first glance keeping a superclass with the "Element" name would be great. I'll think about it.

          4. Difficult to say I was able to catch/foresee all the possible problems. So, even if sort out 3.3 (mentioned above) it will require more testing anyways. I'll email a Bridge NuGet package to you right after posting this message. Maybe, you would you like to give it a try in your project. Also I pushed a new branch with all my changes. You can submit related pull requests (if any) to that branch if you want.
          https://github.com/bridgedotnet/Bridge/tree/issue461

          Comment


            #6
            Let me address your points one-by-one.

            1. I'm glad we're in agreement that CurrentTarget is what is most important. :)

            2. I also agree that it's difficult to decide categorically which generic "signature" is better.. they both have their merits, so whichever you guys think is best will be ok by me.

            3. Now, to address all parts of point 3 (3, 3.1, 3.2, 3.3), I think that we could avoid all of these issues if we ensured that the generic "Element<TCurrentTarget>" class was derived from a non-generic "Element" type, rather than the non-generic class being a specialisation of the generic class (the same with the generic / non-generic "Event" classes). However, I think that the only way to do this is to fall back on to using the "new" keyword on properties that are "overridden" on the generic classes - eg.

            // This is the base Element class, all "CurrentTarget"-related events use the non-generic
            // "Event" class, which specifies this non-generic "Element" class as the CurrentTarget type
            public class Element : Node
            {
                [Name("onkeyup")]
                public Action<Event> OnKeyUp;
                    
                // .. other Element properties go here..
            }
            
            // This is the more specific Element class, any event is overridden (using "new" properties)
            // such that the generic "Event" class is used and so the CurrentTarget references are more
            // strongly typed
            public class Element<TCurrentTarget> : Element where TCurrentTarget : Element
            {
                [Name("onkeyup")]
                public new Action<Event<TCurrentTarget>> OnKeyUp;
            
                // .. other overridden Element properties..
            }
            
            [Ignore]
            [Name("Event")]
            public class Event
            {
                public readonly Element CurrentTarget;
                    
                // .. other Event properties..
            }
            
            [Ignore]
            [Name("Event")]
            public class Event<TCurrentTarget> : Event where TCurrentTarget : Element
            {
                public new readonly TCurrentTarget CurrentTarget;
            
                // .. other overridden Event properties..
            }
            
            // InputElement is derived from the generic "Element" class and so the CurrentTarget reference
            // of the Element events will be of type "InputElement" - however, InputElement is still derived
            // from the non-generic Element class, since InputElement is derived from the generic Element
            // class which is, itself, derived from the NON-generic Element class
            public class InputElement : Element<InputElement>
            {
                public string Value;
            
                // .. other InputElement-specific properties
            }
            Now, I do think that I understand what you meant when you said:

            I also think we will try to avoid using a new modifier on properties when possible.
            To be honest, there's something about "new" used like this that instinctively feels wrong - I think that every good C# Developer gets that niggling feeling when they override properties (or methods) with "new" because different properties / methods are called on the target reference depending upon how that reference is cast. Unlike "genuine" overridden members, which are always called regardless of how the reference is cast.

            But.. there's no getting away from the fact that using "new" is the only way to get "Element<TCurrentTarget>" to inherit from "Element" and to have "Element<TCurrentTarget>" have more strongly-typed properties. And this sidesteps (or so I believe) all of the compilation errors that you highlighted.

            I haven't pushed any code changes to your feature-preview branch, I hoped that the code sample above would illustrate my point. But this might mean that this approach doesn'tsolve every compilation error that crops up. If you find any more (and if you think that this approach is worth exploring) then I will make a more thorough stab at applying my ideas to the real Bridge code base, rather than just outlining an overview of an approach.

            Do you think this makes sense or needs more work.. or is no good at all?

            Comment


              #7
              The approach is definitely worth exploring. I like it. It looks the only unfortunate thing is repeating the properties with a new modifier. Well, probably, not a problem at all with the Event class - there is the only property CurrentTarget that goes to a generic Event<> class. But there are much more in the Element<> class. Say, I change something in a property of a non-generic class (for example, there is a mistype in the docs comment) and I have to remember there is a generic counterpart which I have to apply the same change to. Well, not a very big deal, but not ideal, at least. Though, maybe, there is no better way and the possible benefit of the generic classes is worth enough not to consider that caveat as a showstopper.

              Meanwhile, I've committed the new model. You could play with. Please let me know if you need a NuGet package and I'll email to you.

              Here is a test case I am dealing with.

              Test Case
              using Bridge;
              using Bridge.Html5;
              using System;
              
              namespace Test
              {
                  public class TestGenericElements
                  {
                      // Tests CurrentTarget if it is of required type
                      [Ready]
                      public static void Test1()
                      {
                          InputElement input = new InputElement();
              
                          input.OnChange += (ev) =>
                          {
                              Console.Log("ev.CurrentTarget.Value: " + ev.CurrentTarget.Value);
                          };
              
                          Document.Body.AppendChild(input);
              
                          DivElement div = new DivElement
                          {
                              Id = "div1"
                          };
              
                          div.OnClick += (ev) =>
                          {
                              Console.Log("ev.CurrentTarget is DivElement: " + (ev.CurrentTarget is DivElement));
                          };
              
                          div.AppendChild(new ParagraphElement { InnerHTML = "Paragraph 1" });
                          div.AppendChild(new ParagraphElement { InnerHTML = "Paragraph 2" });
              
                          Document.Body.AppendChild(div);
                      }
              
                      // Tests if Document.GetElementById<> still works
                      // Tests Event's .IsMouseEvent() extension methods
                      [Ready]
                      public static void Test2()
                      {
                          DivElement div = Document.GetElementById<DivElement>("div1");
              
                          div.OnClick += (ev) =>
                          {
                              Console.Log("Document.GetElementById<> works");
                              Console.Log("IsMouseEvent: " + ev.IsMouseEvent());
                          };
                      }
              
                      // Tests if Element is still a superclass of all the element classes and the following code still compiles.
                      [Ready]
                      public static void Test3()
                      {
                          Element element;
              
                          element = new InputElement();
                          element = new TextAreaElement();
                      }
                  }
              }

              Comment


                #8
                If you would send me a NuGet package again, that would be greatly appreciated! Although my colleague has mastered the building-from-source process, I haven't got round to trying it myself yet. A bit lazy, really, I will do before too long because there are times when I'd like to test out other fixes that you guys have committed but not released yet!

                Comment


                  #9
                  I've emailed the NuGet package.

                  As for building from sources it should be quite easy for you once you give it a try:)

                  Comment


                    #10
                    Thanks for that! I've had a quick look at it and I think it looks really good!

                    I've been thinking about it and I can't see any better solution that what you have here, with the "new" property in the generic class. The good thing is that there is only that one, so it shouldn't add too much maintenance burden - personally I think it's a worthwhile trade-off, but I guess the final say isn't up to me! :D

                    I've also tried building from source. I followed the instructions at http://bridge.net/kb/build-from-source/ and that was fine (easy, in fact!).. but I don't know what to do next. How do I start with a simple Class Library project and then Bridge-ify it? I added references to bridge.dll and bridge.html5.dll and removed all of the Microsoft ones, but I presume that I have to set up the Builder somehow? I had a quick look in the NuGet package to see if anything could shine some light on it for me, but I didn't get anywhere. And I tried searching in case you've described this to someone else before, but failed there too.

                    For now, I imagine that I can build custom bridge.dll and bridge.html5.dll binaries and then overwrite the versions in the NuGet package you've provided and then install from that, but I'd like to know how to do it from scratch now that I've started looking at it!

                    Comment


                      #11
                      personally I think it's a worthwhile trade-off
                      Yeah, it really looks so.

                      How do I start with a simple Class Library project and then Bridge-ify it?
                      Could you, please, raise a new forum thread for this one?

                      Comment


                        #12
                        The pull request has been submitted. Once it is merged, we'll get an Event's CurrentTarget to be the underlying Element's type. The goal is to include it to the 1.10 release.
                        Last edited by geoffrey.mcgill; 2015-11-09 @ 05:04 PM.

                        Comment


                          #13
                          The feature has been merged into release1.10 branch.

                          Comment

                          Working...
                          X