Skip to content
This repository has been archived by the owner on Mar 9, 2021. It is now read-only.

Usage of XmlElement over Element is inconvenient #11

Open
ProdigySim opened this issue Jan 8, 2014 · 12 comments
Open

Usage of XmlElement over Element is inconvenient #11

ProdigySim opened this issue Jan 8, 2014 · 12 comments

Comments

@ProdigySim
Copy link
Contributor

Not exactly a bug, but I'm finding the new usage of XmlElement as a return type for various API functions as rather inconvenient, resulting in a lot of manual casting. I understand that these are probably perfectly valid type constraints in terms of the spec, but they're very inconvenient to work with since there are more relevant casts will pass in most common usage.

Some examples:

Element.ParentNode returns XmlElement instead of Element
Document.CreateElement returns XmlElement
Document.DocumentElement returns XmlElement

This means a line like

Document.DocumentElement.AddEventListener("event", ...);

has to turn into

((Element)Document.DocumentElement).AddEventListener("event, ...);

or be separated into two lines, or make use of an extension method to do the cast (which can be more inconvenient given namespace issues).

I don't particularly have a proposed solution, I just feel like the more simplified typing was a lot easier to work with.

@ProdigySim
Copy link
Contributor Author

Also worth noting:

Document.QuerySelectorAll returns XmlNodeList which is particularly inconvenient since it's not even a true IEnumerable.

@n9
Copy link

n9 commented Jan 10, 2014

Another (generated) inconsistency is System.Html.ElementCollection (HTMLCollection) contains XmlElement instead of Element (which would be expectable).

n9 pushed a commit to n9/SaltarelleWeb that referenced this issue Jan 13, 2014
- it looks that order of members has changed (it might need has fixed position)
@erik-kallen
Copy link
Contributor

I do agree with you that this is a problem that needs to be solved. As I wrote in #12, I'm starting to think that the Xml and Html parts should be split between different assemblies. AFAIK, once you're in an HTMLElement tree (eg. $('<xml>')[0] instanceof HTMLElement returns true in the Chrome console.

@erik-kallen
Copy link
Contributor

Please take a look at the latest build and see what you think.

@ProdigySim
Copy link
Contributor Author

I will try to apply this new version to my project as soon as I find time. Thanks.

@n9
Copy link

n9 commented Feb 28, 2014

From my point of view it is ok.

@ProdigySim
Copy link
Contributor Author

I found an hour or so to spend trying to convert my project to the latest Saltarelle.Web from develop. I unfortunately didn't have time to complete the whole conversion--we have a lot of code--but I'll try to finish later.

The improvements from the commits helped, but as you might imagine there's a number of other similar methods with issues. I don't know if you think it's maintainable to try to support them, but I'll list them.

  1. Event.Target is of type EventTarget, which has no direct relation to XmlNode/Element. This necessitates more casting for me in a lot of places. If we could at least assume that an EventTarget was an XmlNode it would be able to pass to a lot of methods.
  2. Element.AppendChild, Element.RemoveChild, and Element.InsertBefore all return XmlNode. Not a huge deal since the return value is rarely used for these.
  3. Event.OffsetX is gone... While this wasn't standard by any stretch, it was supported everywhere but Mozilla. I had some references to it but I won't miss it.
  4. Element.CloneNode returns XmlNode.
  5. Inconsistency between Window.Document and Document. Not a huge deal since usage through window is rare, but references to e.g. Window.Document.Body fail to compile.
  6. Various "List" classes (e.g. ElementCollection, DOMRectList) now use uint for their index parameter, making all for(int i; ...) loops over them require modifications.

It still seems like a fair amount of manual casting has to be done in some cases. I suppose it's easy enough to do a Script.Reinterpret if I'm worried about performance, so it's just about convenience. The few changes you've made have done a lot towards that IMO.

I took some more notes about things I had to convert to go to 3.x. It would be nice if we had the wiki section editable here so I could contribute them to a "Converting to 3.x" article or something.

@n9
Copy link

n9 commented Mar 21, 2014

ad 6) I think it is similar to #14.

@erik-kallen
Copy link
Contributor

  1. EventTarget is the base class of XmlNode / Element, but also of many other classes (eg. System.Data.IndexedDB.Request and System.Html.Media.VTTCue).
  2. Fixed.
  3. I think we can do without it.
  4. Fixed.
  5. Should be fixed (Window.Document was a DocumentBase, not a DocumentInstance).
  6. I just changed the generator to use the int type instead of uint, as well as double over float, as I think this will be more convenient

@erik-kallen
Copy link
Contributor

(commits 88173fb, 88f5351, 9f5b9f9)

@ProdigySim
Copy link
Contributor Author

Those changes should help a lot. I still find #1 in my list to be pretty inconvenient, though. Though, now that I notice HtmlEventHandlerWithTarget, I could probably use that to alleviate a lot of my pain. Hopefully I'll find some time to give the conversion a go again.

@erik-kallen
Copy link
Contributor

I added the wiki page https:/erik-kallen/SaltarelleWeb/wiki/Incompatibilities-between-2.x-and-3.x

Regarding 1, beware that the HtmlEventHandlerWithTarget recieves the element to which the handler is bound as the argument, but Event.Target refers to the original target of the event. Usually you will (probably) want to use the HtmlEventHandlerWithTarget delegate.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants