Menu
▾
▴
[HtmlUnit] [htmlunit:bugs] #1984 Reworking the JS Event listeners implementation
|
From: RBRi <rb...@us...> - 2018-08-21 05:40:19
|
Thanks, another simplification - looks like we are getting closer now. Will have a look at this faiing TypingTest next. --- ** [bugs:#1984] Reworking the JS Event listeners implementation** **Status:** accepted **Group:** Latest SVN **Created:** Tue Aug 14, 2018 01:38 PM UTC by Atsushi Nakagawa **Last Updated:** Tue Aug 21, 2018 03:05 AM UTC **Owner:** RBRi **Attachments:** - [EventListenersContainer.java](https://sourceforge.net/p/htmlunit/bugs/1984/attachment/EventListenersContainer.java) (16.9 kB; application/octet-stream) - [EventListenersContainer.java-2.32.txt](https://sourceforge.net/p/htmlunit/bugs/1984/attachment/EventListenersContainer.java-2.32.txt) (15.5 kB; text/plain) - [EventTarget.java](https://sourceforge.net/p/htmlunit/bugs/1984/attachment/EventTarget.java) (14.7 kB; application/octet-stream) - [EventTarget.java-2.32.txt](https://sourceforge.net/p/htmlunit/bugs/1984/attachment/EventTarget.java-2.32.txt) (13.4 kB; text/plain) ### Problem in brief Thorough testing has revealed event listeners in HtmlUnit are implemented quite liberally in a few places with regards to how they should work. It's somewhat surprising they're working so well in practice. This bug report attempts to address the problems and tackle at fixing them. Scope includes `xyz.addListenerEvent('foo', function () { ... })` listeners and `xyz.onfoo = function () { ... }` property handlers. ##### Problems noticed * Ordering of listeners called during the *at target* phase should honour the order in which the listeners were added, but does not. * Ordering of property handlers such as `window.onload` should honour the timing at which the property was set, with respect to other event listeners, but does not. * Events where `event.bubbles` is `false` should not have a *bubbling* phase, but does. * The load event for `Window`should only traverse `Window` but traverses `Window, Document` instead. * The load event for everything else should traverse from `Document` to all levels down to the element in question, but traverses the element in question only. (Every place using `EventTarget.executeEventLocally()` potentially applies to this and should probably be using the standard `EventTarget.fireEvent()` instead.) * The load event for the `<frame>` element should be treated like "everything else" above, but is not. (On the other hand, the `onload` property for `<body>` and `<frameset>` is correctly tied to `Window`.) * The propagation path of events should not be affected by changes to the DOM tree by intermediate listeners, and therefore the *bubbling* phase should always traverse the same nodes as the *capturing* phase only in reverse, but is and does not. * The return value of event listeners should be ignored (really? it looks that way.. maybe it was different in older IE) and only that of the property handler should be used, but is not. ### Test cases There are five tests which I'm putting in the comments because it's quite long. * `test_onload.html` * `test_frame.html` * `test_click.html` * `test_nested_click.html` * `test_event_return_value.html` These extra two I'm including but haven't fixed yet. They're limited to the load/error behaviour of `<script>` and `<img>` and require fixing up some implementation in `HtmlScript` and `HtmlImage`. * `test_script_onload.html` * `test_img_onload.html` ### Possible fix The fix spans three files: * `html/HtmlPage.java` * `javascript/host/event/EventListenersContainer.java` * `javascript/host/event/EventTarget.java` Changes to `HtmlPage` is the diff below. The changes for `EventListenersContainer.java` and `EventTarget.java` are rather extensive so I'm attaching the modified files along with their respective base versions suffixed with `-2.32.txt`. ```diff --- a/src/main/java/com/gargoylesoftware/htmlunit/html/HtmlPage.java +++ b/src/main/java/com/gargoylesoftware/htmlunit/html/HtmlPage.java @@ -86,15 +86,16 @@ import com.gargoylesoftware.htmlunit.javascript.PostponedAction; import com.gargoylesoftware.htmlunit.javascript.SimpleScriptable; import com.gargoylesoftware.htmlunit.javascript.host.Window; -import com.gargoylesoftware.htmlunit.javascript.host.dom.Node; import com.gargoylesoftware.htmlunit.javascript.host.event.BeforeUnloadEvent; import com.gargoylesoftware.htmlunit.javascript.host.event.Event; +import com.gargoylesoftware.htmlunit.javascript.host.event.EventTarget; import com.gargoylesoftware.htmlunit.javascript.host.html.HTMLDocument; import com.gargoylesoftware.htmlunit.protocol.javascript.JavaScriptURLConnection; import com.gargoylesoftware.htmlunit.util.EncodingSniffer; import com.gargoylesoftware.htmlunit.util.UrlUtils; import net.sourceforge.htmlunit.corejs.javascript.Context; +import net.sourceforge.htmlunit.corejs.javascript.ContextFactory; import net.sourceforge.htmlunit.corejs.javascript.Function; import net.sourceforge.htmlunit.corejs.javascript.Script; import net.sourceforge.htmlunit.corejs.javascript.Scriptable; @@ -1214,18 +1215,28 @@ private boolean executeEventHandlersIfNeeded(final String eventType) { // Execute the specified event on the document element. final WebWindow window = getEnclosingWindow(); if (window.getScriptableObject() instanceof Window) { - final DomElement element = getDocumentElement(); - if (element == null) { // happens for instance if document.documentElement has been removed from parent - return true; - } + // We need the 'Document' node for these load events but getDocumentElement() returns + // <html> (HtmlHtml) which is one below that. The 'Document' node is incidentally just + // us (HtmlPage). (Some tidbits at https://www.w3.org/TR/DOM-Level-3-Events/#event-flow) + final DomNode node = this; final Event event; if (eventType.equals(Event.TYPE_BEFORE_UNLOAD)) { - event = new BeforeUnloadEvent(element, eventType); + event = new BeforeUnloadEvent(node, eventType); } else { - event = new Event(element, eventType); + event = new Event(node, eventType); + } + + // This is the same as DomElement.fireEvent() and was copied + // here so it could be used with HtmlPage. + if (LOG.isDebugEnabled()) { + LOG.debug("Firing " + event); } - final ScriptResult result = element.fireEvent(event); + + final EventTarget jsNode = node.getScriptableObject(); + final ContextFactory cf = ((JavaScriptEngine) getWebClient().getJavaScriptEngine()).getContextFactory(); + final ScriptResult result = cf.call(cx -> jsNode.fireEvent(event)); + if (!isOnbeforeunloadAccepted(this, event, result)) { return false; } @@ -1253,7 +1264,12 @@ private boolean executeEventHandlersIfNeeded(final String eventType) { else { event = new Event(frame, eventType); } - final ScriptResult result = ((Node) frame.getScriptableObject()).executeEventLocally(event); + + // This fires the "load" event for the <frame> element which, like all non-window + // load events, propagates up to Document but not Window. The "load" event for + // <frameset> on the other hand, like that of <body>, is handled above where it is + // fired against Document and directed to Window. + final ScriptResult result = frame.fireEvent(event); if (!isOnbeforeunloadAccepted((HtmlPage) frame.getPage(), event, result)) { return false; } ``` --- Sent from sourceforge.net because htm...@li... is subscribed to https://sourceforge.net/p/htmlunit/bugs/ To unsubscribe from further messages, a project admin can change settings at https://sourceforge.net/p/htmlunit/admin/bugs/options. Or, if this is a mailing list, you can unsubscribe from the mailing list. |
